migrate skaraMirror.sh to be a python script#52
migrate skaraMirror.sh to be a python script#52gdams wants to merge 3 commits intoadoptium:masterfrom
Conversation
gdams
added
pmc-agenda
gdams
force-pushed
the
migration
branch
3 times, most recently
from
7e60998 to
06975b8
Compare
gdams
force-pushed
the
migration
branch
7 times, most recently
from
7ca792f to
423b80e
Compare
sxa
left a comment
sxa
left a comment
There was a problem hiding this comment.
Trying to put down what I said in the PMC call earlier. I'm broadly neutral on this hence this just being a "review comment", but here is some food for thought for anyone else looking at this:
- Since this is a rewrite (and is 538 lines instead of the 319 we have at present for the shell script) there is always a risk of introducing new and exciting bugs
- This would diverge from almost everything else in this space that we have which is written in shell
- Testing is nice, but we could implement suitable tests fairly simply on top of shell scripts, so I'm not sure I see python's
unittestas a compelling reason in itself.
karianna
left a comment
karianna
left a comment
There was a problem hiding this comment.
Some nitpicks but overall this has test coverage in a higher order language and I think that's a good thing.
| @@ -0,0 +1,4 @@ | |||
| [flake8] | |||
| ignore = | |||
| # E501 line too long | |||
There was a problem hiding this comment.
| # E501 line too long | |
| # E501 line too long |
|
|
||
| jobs: | ||
| linter: | ||
| # Name the Job |
There was a problem hiding this comment.
comment in the wrong place and probably not needed
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| # Markdown lint complains about the issue templates | ||
| FILTER_REGEX_EXCLUDE: .github/ISSUE_TEMPLATE/* | ||
| # Lots of shellcheck errors - need fixing |
There was a problem hiding this comment.
Add a TODO or a new issue for this.
| # Apply patches if required | ||
| apply_patches_if_needed(workspace, github_repo) | ||
|
|
||
| # Find the latest and previous release tags that is not in releaseTagExcludeList |
There was a problem hiding this comment.
| # Find the latest and previous release tags that is not in releaseTagExcludeList | |
| # Find the latest and previous release tags that are not in releaseTagExcludeList |
| ) | ||
| newAdoptTags.append(adoptTag) | ||
|
|
||
| if repo.git.rev_parse( |
There was a problem hiding this comment.
Scoping of this if or the print below seems off?
| ): | ||
| print(repo.git.log("--oneline", "origin/release..release")) | ||
|
|
||
| # Find the latest and previous release tags that is not in releaseTagExcludeList |
There was a problem hiding this comment.
| # Find the latest and previous release tags that is not in releaseTagExcludeList | |
| # Find the latest and previous release tags that are not in releaseTagExcludeList |
| remote.fetch(**{"tags": True}) | ||
|
|
||
|
|
||
| def perform_merge_into_release_from_master(workspace, github_repo, branch): |
There was a problem hiding this comment.
Long Python function is Long :-). I think there's some natural places where smaller, well named functions can be extracted
| if os.path.exists(main_workflow_file) and not is_patch_applied( | ||
| main_workflow_file, "- dev" | ||
| ): | ||
| patch_file = os.path.join( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
A block has been put on this Pull Request as this repository is temporarily under a code freeze due to an ongoing release cycle.
If this pull request needs to be merged during the release cycle then please comment /merge and a PMC member will be able to remove the block.
If the code freeze is over you can remove this block by commenting /thaw.
4 participants
This gives us a lot more control over the mirror scripts and allows us to better test the code.
This script was used to generate https://github.com/gdams/jdk22u as a test.