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

Add GemfileLock updater to bump version in Gemfile.lock for Ruby #1768

Closed
andrewthauer opened this issue Dec 6, 2022 · 15 comments · Fixed by #1790
Closed

Add GemfileLock updater to bump version in Gemfile.lock for Ruby #1768

andrewthauer opened this issue Dec 6, 2022 · 15 comments · Fixed by #1790
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@andrewthauer
Copy link
Contributor

andrewthauer commented Dec 6, 2022

Is your feature request related to a problem? Please describe.

I'm trying to use release-please with Ruby gems that have Gemfile.lock committed. The newest official recommendation for this is to commit the lock file (see Should I commit my Gemfile.lock when writing a gem? in the official bundler FAQ). Typically you run bundle install after a version.rb bump which updates a line at the top of the lock file indicating the new version. However, this release-please currently does not do this so CI steps can fail to proceed as the lock file is out of date.

Describe the solution you'd like

I'd like to add a new GemfileLock updater to bump the Gemfile.lock appropriately similar to other lock file updaters in other strategies.

Describe alternatives you've considered

I looked at using the extra-files flag, but since the ruby gem lock file is not a standard format and can't be a template this doesn't seem to be an option.

Additional context

I've started a fork / branch with what I think might be the appropriate changes. I'm not exactly sure the best way to do a full e2e test atm yet though.

@andrewthauer andrewthauer added priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Dec 6, 2022
@schwern
Copy link

schwern commented Dec 6, 2022

I'm having this same problem with every Github release action I've tried.

@andrewthauer Perhaps a simpler and more general approach for release procedures which have to run commands is to allow any commands to be run just before the Release Please changes are committed?

For example...

jobs:
  release-please:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3

      - name: Set up Ruby
        uses: ruby/setup-ruby@v1

      - uses: google-github-actions/release-please-action@v3
        id: release
        with:
          command: manifest
          run_before_commit: |
            bundle install

bundle install would be run after Release Please has done its work, but just before it commits and pushes the PR.

@andrewthauer
Copy link
Contributor Author

@schwern - Thanks! Yes a hook before commit would also work. I had to double check, but I didn't see any option that currently does this. I'm not sure how I would go about adding this atm though. I can dig into the code more unless you can point me in the right direction?

@schwern
Copy link

schwern commented Dec 6, 2022

@andrewthauer Sorry, I haven't looked at the code. It's just a thought I had.

Hooks would be very useful. In addition to before commit, after commit but before push would also be useful. It could run the code in the Release Please branch with Git configured so one could, for example, update the Gemfile.lock in its own commit.

      - uses: google-github-actions/release-please-action@v3
        id: release
        with:
          command: manifest
          before_push: |
            bundle install
            git commit -m 'Update Gemfile.lock'

That would be nice because it would separate the Release Please work from the bundle install work.

@schwern
Copy link

schwern commented Dec 7, 2022

@andrewthauer Do you have a work around for the Gemfile.lock issue that doesn't involve patching Release Please?

@andrewthauer
Copy link
Contributor Author

No, I do not unfortunately. Which is why I attempted to create a fix.

@schwern
Copy link

schwern commented Dec 7, 2022

@andrewthauer I think I have a work around. https://github.com/momentohq/client-sdk-ruby/blob/main/.github/workflows/on-pr-to-main-update-gemfile-lock.yml and you can see it in action here momentohq/client-sdk-ruby#108

This action will fire whenever a PR is opened that changes the Gemfile, version.rb, or gemspec and does not change the Gemfile.lock. It updates the Gemfile.lock and commits. This seems like generally a Good Thing to do for PRs. For more specificity, you can make the action only trigger when a PR is opened with the autorelease: pending label.

The gotcha is that if you run a workflow with the normal GITHUB_TOKEN it will not trigger additional workflows. So the checks will not be re-run on the PR. This is to avoid loops. So you have to use a different token in the action.

@andrewthauer
Copy link
Contributor Author

Interesting, but wouldn't the Gemfile.lock change then be outside the release and git tag? If so, I'm not sure this will work for my purposes.

I like the idea of the hook as a generic solution, but I think my patch would also work and shouldn't cause any issues as the updater will ignore files that do not exist.

Another alternative, seemed to be a plugin, which seem to run after all packages are updated, but before the commits. However, there didn't seem to be much documentation on writing plugins or testing them that I saw.

@chingor13
Copy link
Contributor

It should be pretty trivial to add an updater that knows how to update a Gemfile.lock file. You could then have the ruby strategy attempt to update that file only if it exists.

We would be happy to take that feature if you can help implement, or we will get to it when we have time.

@chingor13
Copy link
Contributor

I've started a fork / branch with what I think might be the appropriate changes. I'm not exactly sure the best way to do a full e2e test atm yet though.

We don't run e2e tests as part of CI, we test the unit test each component and have some compatibility tests for complicated configs.

If you want to test your fork against GitHub, you can compile the CLI in your branch and run it against your repo: https://github.com/googleapis/release-please/blob/main/CONTRIBUTING.md#testing-a-new-feature-using-cli

@schwern
Copy link

schwern commented Dec 7, 2022

Interesting, but wouldn't the Gemfile.lock change then be outside the release and git tag? If so, I'm not sure this will work for my purposes.

@andrewthauer Have another look at the PR. The change to Gemfile.lock is made in the PR branch. Release Please tags and releases after the PR is merged, so the tag and release will include the Gemfile.lock.

I haven't made a release yet to fully test this, but it should work.

I like the idea of the hook as a generic solution, but I think my patch would also work and shouldn't cause any issues as the updater will ignore files that do not exist.

I'm a little skeptical about updating the Gemfile.lock with a regex, but it is a good approximation. Thanks for taking this on, the lack of bundler support in these semantic release actions has been driving me crazy for a month. 👍

@andrewthauer
Copy link
Contributor Author

andrewthauer commented Dec 15, 2022

@schwern - I'll have another try at your workaround. You solution initially wouldn't work because I'm actually triggering a release after every merge to the main branch via some automation using the auto merge feature of GitHub. In this scenario there wasn't a way to hook and do the bundle install. However, I think I might be able to kick off a separate workflow after the release-please PR that then does the bundle install + auto merge. I'll report back once I know.

@chingor13 - Thanks for the information. Even if I can get a workaround working, I think having this as a native feature of release-please makes sense. I'll hopefully have some time in the coming weeks to put up a PR.

@schwern
Copy link

schwern commented Dec 15, 2022

@andrewthauer I realized that since I'm developing a gem, I probably shouldn't commit the Gemfile.lock. 🙄 I know this is against Bundler recommendations, but gem users will have all sorts of different dependencies and I want to reflect that in dev and testing. So that might make my release problems go away.

@andrewthauer
Copy link
Contributor Author

@andrewthauer I realized that since I'm developing a gem, I probably shouldn't commit the Gemfile.lock. 🙄 I know this is against Bundler recommendations, but gem users will have all sorts of different dependencies and I want to reflect that in dev and testing. So that might make my release problems go away.

Actually, it used to be the recommendation to not commit the lockfile but not anymore. See https://bundler.io/guides/faq.html#using-gemfiles-inside-gems (might need to search). We changed over to this, because CI would fail for any gems that are not updated frequently because it pulls in the latest versions. We now commit lock files for gems and have GH create dependency PRs to ensure they are up to date and/or require fixes due to version bumps.

@schwern
Copy link

schwern commented Dec 16, 2022

We changed over to this, because CI would fail for any gems that are not updated frequently because it pulls in the latest versions.

@andrewthauer My thought is that neither using a locked set of dependencies, nor installing the latest deps is correct. Users can have any valid set of dependencies installed. I've been looking into simulating this by installing random, but valid, versions of dependencies in CI, in addition to running with the latest. https://stackoverflow.com/questions/74776586/can-i-install-random-but-valid-dependencies-of-a-gem

For example, I'm pretty sure this gemspec is incorrect. I doubt it will work on grpc 1.0.0. I could manually search for the min dependency, but I'd rather have CI test for it.

@andrewthauer
Copy link
Contributor Author

@chingor13 - I've opened PR #1790 to add support for a Gemfile.lock updater. Since there are are some differences in how ruby parses semvers for prereleases, I added some additional support for that as part of this along with some additional unit test coverage for the existing VersionRB updater as well.

Question. What is the purpose of the -yoshi strategies? Do I also need to update the ruby-yoshi one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants