#1651: push and commit status.json files to ide-urls-status after running the URL update worklfow#2003
Conversation
…s to the new repo ide-urls-status
AdemZarrouki
added
urls
Coverage Report for CI Build 29111555472Coverage decreased (-0.2%) to 72.056%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions72 previously-covered lines in 5 files lost coverage.
Coverage Stats
💛 - Coveralls |
tineff96
left a comment
tineff96
left a comment
There was a problem hiding this comment.
I tested the workflow end-to-end in a fork.
Test setup:
Created a test branch (test/2003-test-branch) in my fork:
tineff96/IDEasy
tineff96/ide-urls
tineff96/ide-urls-status
Adapted the workflow in tineff96/IDEasy to run against the test branch (test/2003-test-branch)
Executed the workflow based on the provided template
(reference run: https://github.com/AdemZarrouki/IDEasy/actions/runs/26902445623)
Result:
The migration of status.json files from ide-urls to ide-urls-status worked as expected.
The workflow correctly commits:
status.jsonfiles are committed toide-urls-status- all other files are committed to
ide-urls
No unexpected files were committed
Important notes for reviewers / future testers:
Setting up the test environment is slightly non-trivial because it requires:
Forks of both repositories (ide-urls and ide-urls-status)
Adjusting the workflow to use the test branch
You must define the following secrets for commits to work:
ACTION_PUSH_TOKEN
BUILD_USER
BUILD_USER_EMAIL
Without BUILD_USER and BUILD_USER_EMAIL, the commit step will fail.
Conclusion:
From my perspective, the implementation behaves correctly and is ready for review.
There was a problem hiding this comment.
@AdemZarrouki thanks for your PR.
Are you aware that the existing code is not only writing the status.json files but also reading them and making choices depending on their content?
IMHO you are currently cloning both repos side-by-side and then running the UpdateInitiator that will not find any status.json without modifying the Java code when they get removed from the ide-urls repo.
I would have expected that you change the Java code to read and write the status.json files now from the separate directory.
If you rather want to do that in the workflow that may also be possible but then you also have to first delete all status.json files from ide-urls anfd then copy them from the new repo before you run the java code. After that you would then move them back and and ensure no status.json remains in ide-urls (what would automatically do the cleanup and delete all these files so we could avoid extra PRs for ide-urls).
However, things to consider:
- Testing workflows is really hard and therefore we should prefer to do as much code in Java and as little in workflows
- I would rather consider this rsync copying, etc. kind of hackish. In the end we have two git repos and a Java process to read and update the data and after that we want to push the changes. When you solve it in Java it IMHO becomes cleaner and the workflow stays shorter and easier to test and understand.
|
@hohwille thanks for the detailed review. So my idea was to avoid the java code change and do instead the changes in the workflow file as this requires less effort, and also i tried to test the workflow on a test branch many times to ensure that the changes are correct. So regarding this:
But if you prefer to change the code so that UpdateInitiator accepts both ide-urls and ide-urls-status, we can do that. |
|
I re-tested the workflow in a test branch after making the changes to the Java code (https://github.com/AdemZarrouki/IDEasy/actions/runs/28228272030). Everything is working as expected. |
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
I still have found some issues.
Since you are not available any more, I will search for someone else from the current team to take over this PR.
| if (args.length == 0) { | ||
| logger.error("Error: Missing path to repository as well as missing timeout as command line arguments."); | ||
| logger.error("Usage: java UpdateInitiator <path_to_repository> <duration_string_format> <tool_to_test|updater_class_name>"); | ||
| logger.error( | ||
| "Usage: java UpdateInitiator <path_to_url_repository> <path_to_status_repository> <duration_string_format> <tool_to_test|updater_class_name>"); | ||
| System.exit(1); | ||
| } | ||
|
|
||
| String pathToRepo = args[0]; | ||
| if (args.length < 2) { | ||
| logger.error("Error: Missing path to URL repository and/or status repository."); | ||
| logger.error( | ||
| "Usage: java UpdateInitiator <path_to_url_repository> <path_to_status_repository> <duration_string_format> <tool_to_test|updater_class_name>"); | ||
| System.exit(1); | ||
| } |
There was a problem hiding this comment.
We do not need two if now.
The first one (if (args.length == 0)) does not make sense any more and shall be removed.
| */ | ||
| public class UrlRepository extends AbstractUrlFolder<UrlTool> { | ||
|
|
||
| private UrlRepository statusRepository; |
There was a problem hiding this comment.
An UrlRepository should not contain another UrlRepository.
BTW: Maybe it was wrong from the start to have the UrlStatusFile in cli module since we never read it from IDEasy itself. It is only used by UrlUpdater and nowhere else.
I would suggest to revert all these changes.
Simply determine the path to the status file from the path to the version folder (from UrlVersion.getPath()).
|
|
||
| private final UrlRepository urlRepository; | ||
|
|
||
| private final UrlRepository statusRepository; |
There was a problem hiding this comment.
Keep this as Path and simply derive the path to the status.json by resolving the relative path inside the repo to the version (e.g. java/java/25.0.3_9) to it plus the constant for "status.json".
I would delete UrlStatusFile.
Also consider moving StatusJson, UrlStatus, and UrlStatusState from cli to url-updater module.
If we need caching of these files, we can simply use a map from path to the parsed StatusJson.
If you want to keep a wrapper object with Path reference and load + save methods also fine but no need to extend that from AbstractUrlFile anymore.
There was a problem hiding this comment.
Also this PR is IMHO still writing to the old locations. Code like this needs to be adopted:
When really deleting UrlStatusFile you will get compile errors revealing the problems to rework.
…status.json-files # Conflicts: # CHANGELOG.adoc
…e-status.json-files # Conflicts: # CHANGELOG.adoc
6 participants
This PR fixes #1651
Implemented changes:
update-urls.ymlto commit and pushstatus.jsonfiles to repoide-urls-statusstatus.jsonshould not be pushed toide-urlsACTION_PUSH_TOKENso that it have access to the new create repoide-urls-status. This need to be done by @hohwille as he has access to that functionality.status.jsonfiles fromide-urls(will be done in Remove status.json from ide-urls after migration #2021).Testing instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
update-urls.ymlto include your test branch. A workflow example was created and it successfully updates bothide-urlsandide-urls-status. https://github.com/AdemZarrouki/IDEasy/actions/runs/26902445623ACTION_PUSH_TOKENand activate the URL Update workflow in your fork.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