-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autobuild: Cleanup analyze_git_references.py #2471
Autobuild: Cleanup analyze_git_references.py #2471
Conversation
The following variable is not referenced in the Autobuild workflow at all and can therefore be safely dropped from the script output: PUSHED_NAME The following variables are referenced in the Autobuild workflow and are declared as outputs, but those outputs are not referred to anywhere. Therefore, they can safely be dropped: - PUSHED_NAME - X_GITHUB_WORKSPACE The following variables are only relevant for releases (PUBLISH_TO_RELEASE=true): - RELEASE_TITLE - RELEASE_TAG - IS_PRERELEASE Therefore, we only need to calculate their value for actual releases. This vastly simplifies the script logic.
What happens if there is a tag without r* on a autobuild branch/non autobuild branch? I’m not sure if we wanted that to build (I‘m unsure if we even use non releatags anymore). |
I've added example output for that as well (all assuming that Jamulus.pro was not updated to match this pseudo-version).
There is no such distinction. A tag is not associated to a branch. It may (and often will) reference a commit which also happens to be on a branch, but that's not relevant for the logic here. What is relevant is the distinction of tags starting with
I guess it could be adjusted, although I don't see what the downside of running a build is there? |
Umm, that's not the complete truth.
|
32f4e09
to
4ced399
Compare
Previously, a push to `rExample` would create a release (due to the tag starting with `r`) while a push to `Example` would not. We should only create (pre-)releases on pushes to `r<Version><OptionalSuffixes>`. This commit ensures that.
4ced399
to
24db7f5
Compare
Updated. I've also added a test case for nightlies to the PR description (@ann0see please also note the comment about the tag format). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and a lot tidier.
@@ -64,7 +64,7 @@ def set_github_variable(varname, varval): | |||
release_version_name = get_release_version_name(jamulus_pro_version) | |||
|
|||
fullref = os.environ['GITHUB_REF'] | |||
publish_to_release = fullref.startswith('refs/tags/r') | |||
publish_to_release = bool(re.match(r'^refs/tags/r\d+_\d+_\d+\S*$', fullref)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to remember using _ instead of . everywhere on git.
(I'm not a fan of two different versions of describing the same version, but as far as I remember it is needed somehow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to remember using _ instead of . everywhere on git.
Yes.
(I'm not a fan of two different versions of describing the same version, but as far as I remember it is needed somehow).
Agreed. I'd be more happy if we were following the more common vA.B.C scheme. If we decide to go that route we should do that properly and adjust all places though. :)
It's just that I think it will cause some breakage (e.g. people checking out git tags have to adjust, scripts may have to be updated, etc.).
If you want to make it happen I suggest opening an issue... :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t know Python well, but I trust your tests (they should include the most commonly used features we need).
Short description of changes
This PR refactors
.github/actions_scripts/analyze_git_references.py
. It drops unused variables and logic, it replaces comments by (hopefully) well-named functions and fixes multiple coding style issues. It also adds a check to fail if we push release tags without properJamulus.pro
VERSION
.Most of the changes are internal to the script and are not supposed to show a change in behavior from the outside.
Some of the changes do have an effect on the output though, which can be seen in the following output variable cleanups:
The following variable is not referenced in the Autobuild workflow at all and can therefore be safely dropped from the script output:
PUSHED_NAME
The following variables are referenced in the Autobuild workflow and are declared as outputs, but those outputs are not referred to anywhere. Therefore, they can safely be dropped:
PUSHED_NAME
X_GITHUB_WORKSPACE
The following variables are only relevant for releases (
PUBLISH_TO_RELEASE=true
). Therefore, we only need to calculate their value for actual releases:RELEASE_TITLE
RELEASE_TAG
IS_PRERELEASE
As I suspect this PR diff is rather hard to read, I'm providing output diffs (before/after this PR, last updated 2022-03-09 11:40 CEST) for the most relevant scenarios as proof that those cases still look sane:
Simulated push to a branch:
Simulated push to a release tag:
Simulated push to a pre-release tag:
Simulated push to a nightly tag:
Note: Releases which use other separators than
_
will no longer be considered releases. This affects some nightly versions (r3.8.0devNightly1
) which have been tagged differently than previously (r3_7_0devNightly1
). I'd like to keep it like that as we should be consistent in our tags (although I'm not opposed to switching from rA_B_C to the more common vA.B.C, but that's out-of-scope for this PR.)Simulated push to a non-version tag starting with r (is this case relevant?):
Simulated push to a non-version tag not starting with r:
CHANGELOG: Autobuild: Improved analyze_git_references.py script
(I'll likely merge all my Autobuild refactoring-related Changelog entries in the end)
Context: Fixes an issue?
Improves readability/maintenance.
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Ready for review.
What is missing until this pull request can be merged?
I'm still planning the following tests on my fork:
Checklist