Skip to content
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

Refactor of dev prerelease auto-update-version workflow #685

Merged
merged 59 commits into from
Apr 22, 2024

Conversation

rashidnhm
Copy link
Collaborator

@rashidnhm rashidnhm commented Apr 16, 2024

Context: Changes to the update_dev_version.yml

Description of the Change:
This PR introduces the following changes to the update_dev_version.yml workflow:

  • Refactor dev_version_script.py to use python-semver
    • Why? -> It is generally more robust to parse and bump versions. In the previous version, it was assumed that the version on the PR was the same as master, then the pr version got bumped by 1. Though that is true for 99% of the cases, it left out a chance for a bad version to get by. The check is a bit more strict now, where the version is taken from master, bumped up by 1, then checked against the pr version.

Example of this in action:

  • Create a branch off of master
  • As of writing, the version in master is 0.36.0-dev29
  • In your branch, update the version manually to 0.36.0-dev25 (Simulates someone accidentally drifting away from the master version by more than 1)

With the previous version of the workflow, the pr would get auto updated to: 0.36.0-dev26. This is technically invalid as that version already exists. Though it can be caught during manual PR review it might get by as well.

With the new workflow, the pr would get auto updated to: 0.36.0-dev30.

  • Change update_dev_version.yml to run on pull_request_target
    • Why? -> This would let the action trigger from the master branch instead of the PR branch, which means it will have access to secrets (even if the PR is from a fork!) and would be able to push to PRs as ringo bot. Usage of the token also means that when a new commit is pushed, new CI will be kicked off.

Other notable changes:

  • The commit pushed by the bot now includes the version it bumped from/to.
    image
    image

Benefits:
Noted in the section above.

Possible Drawbacks:
With CI triggering itself, there is always a possibility CI recursively calling itself infinitely. The base case here is if the version matches what is expected, then nothing is pushed to the PR.

Related GitHub Issues:
None. sc-61382

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@rashidnhm rashidnhm force-pushed the sc-63182-version-update branch from b8a898e to b44aaa8 Compare April 16, 2024 19:05
@rashidnhm rashidnhm requested review from mlxd and chaeyeunpark April 17, 2024 16:11
@rashidnhm rashidnhm marked this pull request as ready for review April 17, 2024 16:11
@rashidnhm rashidnhm removed the request for review from chaeyeunpark April 17, 2024 16:51
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rashidnhm
Just a few quick questions

.github/workflows/dev_version_script.py Show resolved Hide resolved
.github/workflows/update_dev_version.yml Outdated Show resolved Hide resolved
.github/workflows/update_dev_version.yml Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Show resolved Hide resolved
@mlxd mlxd requested a review from maliasadi April 17, 2024 16:58
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 🥳

.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
.github/workflows/dev_version_script.py Outdated Show resolved Hide resolved
@rashidnhm rashidnhm requested review from mlxd and maliasadi April 18, 2024 16:14
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rashidnhm
No further issues from my end

Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rashidnhm 💯

@mlxd mlxd added the ci:build_wheels Activate wheel building. label Apr 18, 2024
@rashidnhm rashidnhm merged commit 7ff0b52 into master Apr 22, 2024
82 checks passed
@rashidnhm rashidnhm deleted the sc-63182-version-update branch April 22, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:build_wheels Activate wheel building.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants