#1651: push and commit status.json files to ide-urls-status after running the URL update workflow - #2146
Conversation
Coverage Report for CI Build 32352409678Coverage increased (+0.7%) to 73.644%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions97 previously-covered lines in 7 files lost coverage.
Coverage Stats💛 - Coveralls |
d48c394 to
b55bccf
Compare
68f1eaf to
b136f67
Compare
maybeec
left a comment
There was a problem hiding this comment.
Thanks for picking this back up from #2003 👍 This is a clean split: status.json handling moves out of the cli URL model into url-updater, the UrlStatusFile/StatusJson/UrlStatus/UrlStatusState classes move with it, and the GHA workflow now checks out, updates and pushes both ide-urls and the new ide-urls-status repo. Nice that the old UpdateManager(Path, UrlFinalReport, Instant) constructor was kept and just delegates to the new one — BuildSecurityJsonFiles in the security module still compiles against it unchanged, which is exactly the right way to avoid a breaking signature change.
Against issue #1651's requirements: workflow checks out the new repo ✅, UrlUpdaters write status files into the new repo's mirrored folder structure ✅, workflow commits/pushes both repos ✅. One item from the issue doesn't look addressed here: "We need a new access token to also include write access to the new repo" — see the inline comment on the workflow file.
A few should-fix points below, nothing blocking. Also two small housekeeping notes: the PR is currently shown as behind main by GitHub, so please rebase/update before merging; and the PR title has a small typo ("worklfow" → "workflow").
539dce1 to
c70b2b8
Compare
There was a problem hiding this comment.
@laert-ll thank you for taking over this story. You perfectly catched up the requirements from story and review remarks to bring this to the finishing line. 👍
My remarks are all about managing and saving the new status.json files and the main question I have is, whether after merging the current state, it could happen that we modify a status file in memory but forget to write it to disk what would be a bug causing potential problems. If this is not the case and we finally want to get this done, we can also create a cleanup ticket and merge this PR as is and do the cleanup later when we have time for it.
If you think you can address the review comments quickly and prefer to include them into this PR that is also fine even in case there is no bug.
747cd27 to
004626a
Compare
This PR fixes #1651
Continuation of PR#2003
Implemented changes:
status.jsonhandling out of thecliURL model into theurl-updatermodule:StatusJson,UrlStatusandUrlStatusStateare relocated, and the oldAbstractUrlFile-basedUrlStatusFileis replaced by a lightweightPath-based wrapper.status.jsonpath is now derived from the version folder instead of maintaining a parallelUrlRepositorytree; the status repository is passed around as a plainPath.UpdateManager/UpdateInitiatoraccept the separateide-urls-statusrepository, and theupdate-urlsworkflow checks it out and pushes to it.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal