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

Reduce bundler version hardcoding #4973

Merged
merged 3 commits into from
Aug 3, 2022

Conversation

deivid-rodriguez
Copy link
Contributor

This is an experiment to reduce the number of places that need to be updated when Bundler version is bumped to just the Dockerfile and the bundler/helpers/v2/Gemfile.lock files.

@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch from d60ffbb to ccf1e06 Compare April 7, 2022 17:44
@mattt
Copy link
Contributor

mattt commented Apr 21, 2022

Thanks for working on this, @deivid-rodriguez! I just opened #5024, which I hope to help DRY up a lot of our infrastructure. Do you see any potential issues / opportunities with this new approach?

@deivid-rodriguez
Copy link
Contributor Author

@mattt Unfortunately my idea here didn't work as well as I expected, because of what I think it's a chicken-and-egg issue. I wanted to let RubyGems (required first thing when booting Ruby) choose the appropriate Bundler version according to the version in the Gemfile.lock file, instead of having to reimplement that logic ourselves. But it turns out we need to know the Bundler version beforehand, so that we can apply the proper monkey patches to Bundler, so that dependabot works properly. I left the PR opened because I want to have another look, but it my initial approach didn't really work well.

I'll have a look at the new infra to see if I come up with new dry up ideas, but don't worry at all about how it could affect this cleanup since it's already not a successful attempt :)

@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch from ccf1e06 to 84208c4 Compare April 22, 2022 09:28
@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch 5 times, most recently from 0372ade to de0e7e5 Compare April 22, 2022 15:46
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review April 22, 2022 15:46
@deivid-rodriguez deivid-rodriguez requested a review from a team April 22, 2022 15:46
@deivid-rodriguez
Copy link
Contributor Author

Alright, I found a way to do this.

It doesn't allow to remove Dependabot helpers to parse lockfiles, which was one original motivation from #4884 (comment).

But it does accomplish the goal of having a single source of truth for the Bundler version run by Dependabot (well, two, actually, but much better than before :)).

@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch from de0e7e5 to e8b9d12 Compare April 22, 2022 16:12
@deivid-rodriguez deivid-rodriguez marked this pull request as draft April 22, 2022 18:10
@deivid-rodriguez
Copy link
Contributor Author

I got one more idea to potentially further improve this and leave the Bundler version in a single place, and a much simpler setup. Will give it a try tomorrow.

@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch 4 times, most recently from 83d51ec to deafea3 Compare April 24, 2022 16:39
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review April 24, 2022 17:18
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner April 24, 2022 17:18
@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch from deafea3 to 9eafa96 Compare April 24, 2022 19:29
@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Apr 25, 2022

Ok, I put together a small patch that leaves the exact Bundler version at a single place (the Dockerfile). The removal of harcoded versions is based on two ideas:

  • Bundler by default runs the highest version installed the image. That's 2.3.x. There's no need to explicit pass BUNDLER_VERSION for running tests and building helpers since that's default default behavior (it's still needed though to force Bundler 1.17.3).
  • When shelling out to native helpers, we don't need to know the exact version, we just need to know the major version to figure out the path to native helpers run.rb script and everything else. Proper activation of bundler can be done inside the run.rb script without needing the full version either.

@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch 2 times, most recently from cb83965 to e836b8a Compare July 14, 2022 17:53
@jeffwidman
Copy link
Member

@deivid-rodriguez is this ready for review or still a work-in-progress?

There's only two versions of Bundler installed (1.17.3, and 2.3.x), and
RubyGems chooses the highest version by default. So there's no need to
explicitly pin the version when we want to use Bundler 2.3.x.
This note makes total sense for `helpers/v1/build`, because there we
explicitly pass `BUNDLER_VERSION=1.17.3` to `bundle install`, which
generates a `Gemfile.lock` using that version, which indeed forces
RubyGems into choosing Bundler 1.17.3 instead of the highest version
installed.

Here, however, we would get the same behaviour of 2.3.18 being chosen,
with or without a `Gemfile.lock` using that, because it's the highest
version.
When shelling out to native helpers, we don't need to know the exact
version, we just need to know the major version to figure out the path
to native helpers run.rb script and everything else. Proper activation
of bundler can be done inside the run.rb script without needing the full
version either.
@deivid-rodriguez deivid-rodriguez force-pushed the reduce-bundler-hardcoding branch from e836b8a to 0412766 Compare August 1, 2022 16:56
@deivid-rodriguez
Copy link
Contributor Author

Yes! I only changed some commit messages to try to better explain the approach I ended up taking, but other than that I think it's ready!

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

This all makes sense to me, thank you for the helpful commit messages, they made it much easier to follow the logic.

I'm 👍 for merging this, but I don't have time to cut a release and deploy it just yet, and I don't want to push that risk onto a teammate w/o their permission in case there's a problem and they need to revert. I'll see if someone else can take a look, otherwise will merge when I have the space to also deploy it.

@deivid-rodriguez
Copy link
Contributor Author

That sounds super wise @jeffwidman 👍, thanks!

@jurre
Copy link
Member

jurre commented Aug 3, 2022

Yes, I think this should work, and is a super welcome simplification! 🎉

@jurre jurre merged commit f614de4 into dependabot:main Aug 3, 2022
@deivid-rodriguez deivid-rodriguez deleted the reduce-bundler-hardcoding branch August 3, 2022 20:54
@deivid-rodriguez
Copy link
Contributor Author

Let me know of any issues 🤞

@jeffwidman
Copy link
Member

Thx again for this @deivid-rodriguez, a really nice simplification.

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.

4 participants