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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daeb3be
Setup steps after release-please to automate gem version syncing
carlosantoniodasilva May 3, 2024
4c06977
Checkout the PR head branch name where we need to sync
carlosantoniodasilva May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,28 @@ 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.

with:
ref: ${{ steps.release.outputs.pr.headBranchName }}

- name: Sync versions - setup Ruby
if: ${{ steps.release.outputs.release_created }}
uses: ruby/setup-ruby@v1
with:
ruby-version: 3.3

- name: Sync versions
if: ${{ steps.release.outputs.release_created }}
run: |
bin/sync-versions
git config user.name github-actions[bot]
git config user.email 41898282+github-actions[bot]@users.noreply.github.com
git add .
git commit -m "Sync versions"
git push

publish:
name: Publish to Rubygems
needs: release-please
Expand All @@ -29,12 +51,12 @@ jobs:

steps:
- name: Checkout code
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Setup Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: 3.2.2
ruby-version: 3.3

- name: Publish gems
env:
Expand Down