-
Notifications
You must be signed in to change notification settings - Fork 26
Implement Schema Version Update Automation #276
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: ImJustHenry <[email protected]>
Signed-off-by: ImJustHenry <[email protected]>
|
Thanks @ImJustHenry for your contribution! A couple of comments:
|
xibz
left a comment
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.
Thanks for the PR! Overall it looks good, but just had a few minor comments.
| return major, minor, patch, suffix | ||
|
|
||
|
|
||
| def bump_version(major, minor, patch): |
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.
nit: maybe instead we could have just 3 new variables, and we should probably throw an exception for unknown LABELs
new_patch = 0
new_minor = 0
new_major = 0
if LABEL == "patch":
new_patch = patch + 1
elif LABEL == "minor":
new_minor = minor + 1
elif LABEL == "major":
new_major = major + 1
else
raise ExceptionType(f"invalid LABEL: {LABEL}")
return new_major, new_minor, new_patch
Additionally, we may need a suffix or prefix potentially, e.g. "v1.0.0-beta". But I think that can come when we need it. It looks like you started some functionality for suffix, but I dont think it's done.
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.
Thanks you for the feedback!
I understand the suggestion to use three new variables, it does make the bump logic more explicit and clear. I also see your point about throwing an exception for unknown labels. In the current setup, the GitHub workflow only passes predefined labels (patch, minor, major, release), so in practice an exception isn’t strictly necessary.
That said, adding an exception could serve as a useful safeguard if someone were to run the script manually with an invalid label. I’ll update the script to incorporate both of these suggestions.
| old_version = read_version() | ||
|
|
||
| # Parse current version | ||
| major, minor, patch, suffix = parse_version(old_version) |
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.
What happens to suffix?
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.
Thanks for the question! The suffix is parsed so that I can correctly separate the numeric patch value from any trailing suffix such as -draft. I designed the logic under the assumption that versions should always include -draft unless the label is release, in which case the suffix is intentionally removed.
So the suffix is extracted purely so the script can reliably parse versions like 0.5.0-draft without errors, and so a release label can strip the suffix. Outside of that case, the script intentionally replaces the suffix with -draft on all non-release bumps.
| else: | ||
| # Otherwise, bump version according to label | ||
| new_major, new_minor, new_patch = bump_version(major, minor, patch) | ||
| new_version = f"{new_major}.{new_minor}.{new_patch}-draft" |
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.
Can't we use suffix here?
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.
As mentioned, the original assumption was that only finalized releases should omit the -draft suffix. That’s why I intentionally append -draft during version bumps, so all non-release versions are clearly marked.
If the goal is to preserve existing suffixes (potentially for more complex cases in the future like -beta) the logic can certainly be updated to consistently use the parsed suffix instead of forcing -draft. The current implementation mainly ensures that non-release versions are easily distinguishable from official releases.
|
@afrittoli - The shell script seems a little larger in scope. This doesn't do version file replacement in JSON, for example, but to manage the version specifically. I think we can use the shell script and this in conjunction. So I think we will need to update the shell script as well or move the rest of it to the python script. @ImJustHenry - Can you take a look at that shell script? I think we may be able to utilize both of the files to do a release, or we can completely replace the shell script with full python, but that requires doing the file version replacement to also be added. |
|
The main problem I see with this approach is that it will attempt to create a new PR to update versions every time a PR is labelled. I think we should execute this at release time, not on every PR. Even if we wanted to update versions every PR, we shouldn't do it with an extra PR, there would be no easy way to coordinate merging the PRs in the right order then.
But again, I don't think we should update versions for every single PR. @xibz @davidB thoughts? |
|
@xibz - I’ve reviewed the shell script. Please correct me if I miss anything but the shell script basically manages releases by updating all version references across the repository, including JSON schemas, conformance files, and documentation, based on the version specified in version.txt. The approach could be that we can utilize both scripts, the Python script calculates the next version based on the label and updates version.txt, while the shell script reads version.txt and updates all references across the repository. Or alternatively, if you prefer having everything in a single language, I can convert the shell script’s functionality into the Python script. I’m happy to proceed with whichever approach is preferred. @afrittoli - Thank you for the feedback! I actually modeled the workflow after what I observed with Dependabot, which creates a new PR every time it bumps a dependency version. My assumption was that each merged PR into main would correspond to either a minor, major, or patch change, and thought that the repository should have a PR for every version commit for consistency. I understand your concern about creating a PR on every labeled PR and the potential coordination issues. To address this, the approach you mentioned of using manual dispatch to create releases makes sense. This way it does ensure that the version bumps only happen intentionally, instead of triggering on every PR. I can start updating the code and incorporate all the feedback if there are no other issues that haven’t been addressed. |
Changes
tools/bump_version.py) to automate version bumps based on PR labels (patch,minor,major,release)..github/workflows/version-bump.yml) that triggers on PR labeling.releaselabels, the workflow removes the-draftsuffix from the version.-draft) while incrementing the appropriate version component.Related Issue
Partially fixes #218
Submitter Checklist
As the author of this PR, please check off the items in this checklist: