-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Bump bundler from 2.2.20 to 2.2.25 #4132
Conversation
7a2129f
to
29bf313
Compare
https://github.com/rubygems/rubygems/blob/master/bundler/CHANGELOG.md#2225-july-30-2021 Something we _might_ get some support questions on is: rubygems/rubygems#4647 but overall I think that's a very good change! The GEM sources not being split has bitten our users in the past, so happy to see that change.
29bf313
to
b544c92
Compare
@@ -31,7 +31,7 @@ def self.detected_bundler_version(lockfile) | |||
if (matches = lockfile.content.match(BUNDLER_MAJOR_VERSION_REGEX)) | |||
matches[:version] | |||
else | |||
FAILOVER | |||
"1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Should we remove the FAILOVER
constant or change it just to equal 1
? Or should it always be 1.173
instead? Are there any scenarios where passing the full major version would cause problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use it in two separate places, this detected_bundler_version
method is used to gather metrics on the bundler major version, but in bundler_version
we now need the full version.
BUNDLER_VERSION=1 bundle install | ||
BUNDLER_VERSION=1.17.3 bundle config set --local path ".bundle" | ||
BUNDLER_VERSION=1.17.3 bundle config set --local without "test" | ||
BUNDLER_VERSION=1.17.3 bundle install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should smooth the process of updating bundler in future by having these values in text files we can read in various places?
It might be worth that or a helper script to do bundler updates, but given we have to run things in different bundler contexts that might get tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually started on a script, but the only approach I can come up with is pretty crude and brittle, along the lines of:
uri = URI.parse("https://rubygems.org/api/v1/versions/bundler.json")
response = Net::HTTP.get_response(uri)
body = JSON.parse(response.body)
versions = body.map { |d| Gem::Version.new(d["number"]) }
latest_v2_release = versions.filter { |d| d.canonical_segments.first == 2 }.max
latest_v1_release = versions.filter { |d| d.canonical_segments.first == 1 }.max
dockerfile = File.read("Dockerfile")
dockerfile.gsub!(/\&\& gem install bundler -v 1.+$/, %(&& gem install bundler -v #{latest_v1_release} --no-document \\))
dockerfile.gsub!(/\&\& gem install bundler -v 2.+$/, %(&& gem install bundler -v #{latest_v2_release} --no-document \\))
IO.write("Dockerfile", dockerfile)
I'm not sure if such a script is worth maintaining, as we'll also need to update it as our code changes, I'd expect changes to the script would end up taking roughly as much time as going through the files?
There might be a better approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I agree that making that maintainable and future proof isn't free compared to a find-and-replace. I don't think it should block this change to solve that either, we might not change this often enough it's worth the metaprogramming 🤔
b544c92
to
852779c
Compare
In a recent version of bundler, passing a major version like `BUNDLER_VERSION=2 bundle --version` no longer works. I suspect this may have happened here: https://github.com/rubygems/rubygems/pull/4750/files We now need to pass the exact version of bundler we're using, which is a bit cumbersome as you can see from the diff, and it also means we'll have to change a few more lines every time we update a version of bundler. I don't think the current behavior we were relying on was documented, so I'm not sure if we can open an issue with the Bundler team. This seems like the most straight forward fix to me, but I am definitely open to other suggestions
852779c
to
5a5b555
Compare
`lock_shared_dependencies` was renamed to `conservative` in rubygems/rubygems@fb4a8a5
c8713e9
to
5593ca8
Compare
https://github.com/rubygems/rubygems/blob/master/bundler/CHANGELOG.md#2225-july-30-2021
Something we might get some support questions on is:
rubygems/rubygems#4647 but overall I think
that's a very good change! The GEM sources not being split has bitten
our users in the past, so happy to see that change.
In a recent version of bundler, passing a major version like
BUNDLER_VERSION=2 bundle --version
no longer works.I suspect this may have happened here: https://github.com/rubygems/rubygems/pull/4750/files
We now need to pass the exact version of bundler we're using, which is a
bit cumbersome as you can see from the diff, and it also means we'll
have to change a few more lines every time we update a version of
bundler.
I don't think the current behavior we were relying on was documented, so
I'm not sure if we can open an issue with the Bundler team.
This seems like the most straight forward fix to me, but I am definitely
open to other suggestions