Bump bundler from 2.3.8 to 2.3.9 and automate it#4884
Conversation
deivid-rodriguez
left a comment
There was a problem hiding this comment.
Looks good, and unlikely to have false positives I believe. I think it's only missing updates in bundler/script/ci-test.
I left a couple of comments with suggestions on how to reduce the number of places that need updates in the future, just something to consider in future improvements.
| module Helpers | ||
| V1 = "1.17.3" | ||
| V2 = "2.3.8" | ||
| V2 = "2.3.9" |
There was a problem hiding this comment.
The Bundler version selection behavior implemented in this file should be the default as long as there's single v1 and v2 versions installed in the running environment. If that's the case I think it should be possible to completely remove this logic (don't pass BUNDLER_VERSION at all to native helpers) and let RubyGems do the right thing. But it seems something to consider for the future, definitely not for this PR.
There was a problem hiding this comment.
Aye, yeah we should definitely try that and see if it doesn't break anything!
|
|
||
| let(:v1) { "1.17.3" } | ||
| let(:v2) { "2.3.8" } | ||
| let(:v2) { "2.3.9" } |
There was a problem hiding this comment.
I think this spec could be relaxed to not hardcode exact versions, but instead just check the major version selected.
There was a problem hiding this comment.
I'm not sure, right now I think the exact version is relevant, if we do this then we can maybe relax it?
There was a problem hiding this comment.
These specs are unit tests for the Bundler::Helpers.bundler_version method, which selects an exact bundler version according to the lockfile contents, using the following criteria:
- No lockfile -> The exact v2 version dependabot has available.
- Lockfile with no BUNDLED WITH section -> The exact v2 version dependabot has available.
- Lockfile with BUNDLED WITH 1.x section -> The exact v1 version dependabot has available.
- Lockfie with BUNDLED WITH 2.x section or higher -> The exact v2 version dependabot has available.
Namely, a unit test for this method
dependabot-core/bundler/lib/dependabot/bundler/helpers.rb
Lines 18 to 26 in ea8fbaf
In my opinion, testing that the result is a valid 3 component version number, and that the first segment is correct (1.x or 2.x) is enough. The exact version itself that dependabot is using is "configuration" and should not need a test, because it's not useful for detecting bugs and it means essentially testing the specific value of a constant, which defeats one of the points of using constants (keep things DRY).
There was a problem hiding this comment.
That said that just my personal opinion but it's no big deal, and indeed if #4884 (comment) works, all this code will go away, so these tests will go too anways.
b43df56 to
c2f041a
Compare
|
Looks good to me! |
This bumps the bundler version to the latest release, and adds a quick 'n dirty script to do it for us going forward. This solution isn't ideal as places where we reference the version might change, but I think it's an improvement over manually going through the files.
c2f041a to
575147c
Compare
landongrindheim
left a comment
There was a problem hiding this comment.
Thanks for checking in the automation! Looks good to me 🚀
Outside the scope of this change: Do you think we could define the Bundler version in one place as a constant and reference it throughout? I'm thinking we could have Dependabot keep it up-to-date 😄
I think it would be tricky, because we reference it in the Dockerfile, in Ruby source and bash scripts, the only things I can think of are real hacks 🤔 |
Same here. If I think of something I'll raise it with you 😄 |
This bumps the bundler version to the latest release, and adds a quick
'n dirty script to do it for us going forward.
This solution isn't ideal as places where we reference the version might
change, but I think it's an improvement over manually going through the
files. Also, if we happen to reference the same value as the bundler version
to mean something else in the same file, it'll get updated.