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

Setup steps after release-please to automate gem version syncing #208

Closed
wants to merge 2 commits into from

Conversation

carlosantoniodasilva
Copy link
Member

This is an attempt to automatically sync our gem versions after release-please detects the need to bump the version. It will open up a PR with that new version bump to judoscale-ruby and changelog added, and these extra steps should follow-up that with syncing the new version across the other libraries.

I have done some manual testing on this using a separate branch, by commenting out the release-please and "if" statements, and only triggering these actual steps that sync versions and push a new commit. By bumping the judoscale-ruby version myself in a commit, the automated steps here would generate a new follow-up commit automatically syncing the other gems versions, so I'm hopeful this will work as expected. (famous last words)

Screenshot 2024-05-03 at 13 23 57

The git user name/email and commands in general are based on examples:
https://github.com/actions/checkout?tab=readme-ov-file#push-a-commit-using-the-built-in-token
https://github.com/google-github-actions/release-please-action/tree/v3.7.13?tab=readme-ov-file#creating-majorminor-tags


Here's my original message sharing the steps I took for testing, for reference:

I pushed a commit with those steps to a branch, commenting out the rest of the release-please stuff, and updating the version, and it added a new commit syncing correctly. I pushed one more version bump alone, and it synced again. Here's the latest action that synced.

Another idea I was contemplating was to try a pre-commit hook, but since this is just running on those release PRs, and we'll squash the commits anyway, this might be all we need.

This is an attempt to automatically sync our gem versions after
release-please detects the need to bump the version. It will open up a
PR with that new version bump to judoscale-ruby and changelog added, and
these extra steps should follow-up that with syncing the new version
across the other libraries.

I have done some manual testing on this using a separate branch, by
commenting out the release-please and "if" statements, and only
triggering these actual steps that sync versions and push a new commit.
By bumping the judoscale-ruby version myself in a commit, the automated
steps here would generate a new follow-up commit automatically syncing
the other gems versions, so I'm hopeful this will work as expected.
(famous last words)

The git user name/email and commands in general are based on examples:
https://github.com/actions/checkout?tab=readme-ov-file#push-a-commit-using-the-built-in-token
https://github.com/google-github-actions/release-please-action/tree/v3.7.13?tab=readme-ov-file#creating-majorminor-tags
@@ -21,6 +21,26 @@ jobs:
bump-minor-pre-major: true
version-file: "judoscale-ruby/lib/judoscale/version.rb"

- name: Sync versions - checkout code
if: ${{ steps.release.outputs.release_created }}
uses: actions/checkout@v4
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we'll have to do anything here to checkout the newly-created release PR branch. The action (workflow?) itself is running on main, so my guess is that the checkout and commit will also happen on main.

In your testing, I think the action was running on the branch, so the automated commit was also on the branch.

If I'm right about this, then maybe we'll want a separate action that only triggers on release PR's, and all that action does is sync the versions.

Not sure how to test any of this, though, without pushing to main and creating a release PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you're right, I didn't think this through with the different branch it runs vs the one it creates. It couldn't be that simple 😅

It seems the "outputs" object has access to a PR object if one was created (which it'd have been in this case), and that PR object has access to both head & base branch names: https://github.com/googleapis/release-please/blob/cb106cd0b5ae5c3e2a3b015487dc615eba72b842/src/pull-request.ts#L15-L17, so maybe we can checkout the right branch for that PR and run the sync/commit there?

I think that'd mean set the ref attribute according to actions/checkout docs, to something like steps.release.outputs.pr.headBranchName. It's definitely hard to test all this, without triggering the real thing!

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed that change, but I am now also thinking (and this may be a bit controversial): what if we removed the VERSION for each gem, and let the gemspec use Judoscale::VERSION via require_relative ../judoscale-ruby/.../version.rb

Since we're treating the version the same anyway, it's in theory not necessary to have that constant on each gem (it's more of a common thing, not a requirement). The gemspec is also built via checking out the entire source code so it has access to judoscale-ruby, and the gemspec doesn't need to be included in the code that's shipped with the gem. (it's statically built locally when pushing the *.gem)

It might make things more difficult if we ever go back from generating the same version across the board, though. (but then we wouldn't have any automated syncing anyway) We also report that VERSION as adapter version to the API, so there's that.

Anyway, food for thought.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that's controversial at all! It sounds much more reasonable than what we're doing now. 😂

Let's do that!!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, let's give it a shot.

I pushed a separate PR with that change #210, since I was wrapping up some other gemspec improvements already, so pushed a PR on top of that to avoid conflicts. Will close this one for now.

We need to checkout the release PR to sync versions, otherwise it might
use `main` which is where this workflow is running.

It seems the "outputs" object has access to a PR object if one was
created (which it'd have been in this case):
https://github.com/google-github-actions/release-please-action?tab=readme-ov-file#outputs

and that PR object has access to both head & base branch names:
https://github.com/googleapis/release-please/blob/cb106cd0b5ae5c3e2a3b015487dc615eba72b842/src/pull-request.ts#L15-L17

so I believe we can checkout the right branch for that PR and run the
sync/commit there, with this change.

It's hard to test all of this without actually running a release though,
so we might have to wait and see! (and hope!)
@carlosantoniodasilva carlosantoniodasilva deleted the ca-sync-gem-versions-pr branch May 6, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants