dependabot
Stop running Bundler v1
What are you trying to accomplish?
Stop running Bundler v1 internally, and thus dropping all Bundler v1 specific code, without dropping support for providing updates for Bundler v1 lockfiles.
Anything you want to highlight for special attention from reviewers?
To do this, I adapted native helpers to pass an explicitly requirement on the locked version of Bundler to the resolver. With that in place, the end user behavior should still be the same. Ironically, the changes caused some trouble in Bundler 2. I think it should be possible to reconcile that, but for now I only changed helpers when locked bundler version is v1, and left helpers unchanged otherwise. It seemed also a plus to make this less risky to not change anything for Bundler v2.
Also, I'm pretty sure @jurre will ask about the following so I'll explain. There's some stuff that may change in edge cases. For example, if v1 lockfile uses multiple global sources (something considered insecure), Dependabot will now start splitting the gems in the lockfile by the source they come from. I'm not sure how Bundler 1 deals with that, and since Dependabot currently does not modify the BUNDLED WITH section in the lockfile, it may cause some trouble.
I think the above is acceptable given Bundler 1 is very old, and only particular cases like that may be affected, but generally updates should still work fine!
How will you know you've accomplished your goal?
Bundler v1 users can still get Dependabot PRs, and at the same time we can get rid of all the Bundler v1 specific code.
Checklist
- [x] I have run the complete test suite to ensure all tests and linters pass.
- [ ] I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
- [x] I have written clear and descriptive commit messages.
- [x] I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
- [x] I have ensured that the code is well-documented and easy to understand.
Probably worth doing some realworld testing, but a green spec suite, both with bundler 1 and bundler 2, is reassuring!
Regarding spec changes, I only changed expectations when I considered the new behavior in Bundler 2 better. When it was not clear, I fixed things so that things still work as the work today.
Thanks David! Should we consider just dropping support for bundler v1 altogether instead?
I don't know but this is ready, so shouldn't be necessary to make that decision now if you don't want to!
I'm mostly thinking if we just drop support, we don't need to worry about potential edge-cases or patch up any issues we find with how this works against real world bundler 1 projects.
🤷♂️ Up to you! I'm just proposing this because the previous times dropping support was considered, the number of users was still not small enough to justify a hard drop. Since the main annoyance of supporting Bundler v1 is maintaining a doubled code base, I figured we can fix the main annoyance and let users keep gradually moving on without disruption.
Maybe ship this and announce official Bundler v1 deprecation at the same time, announcing that while support is still there, internal changes my cause issues for some users that won't necessarily be addressed.
@jurre I'm not able to deploy this without your approval. Please take a look when you get a chance.
To be entirely honest, I feel a little unsure about this change, David captured my reservations well in the PR body.
I would much rather we just push to stop supporting Bundler v1 altogether, rather than have potentially broken support out there. My worry is that, yes it might work for people using a single source, but the potential support burden from people who are using multiple sources seems not worth the risk. If we're not committed to fully supporting Bundler 1, and to be candid, I am not, I'd rather we just drop support entirely and communicate that clearly to our customers.
As mentioned, Bundler v1 is very old, usage is low, the upgrade path is really good these days. Why should we put ourselves in this awkward situation?
cc @jonjanego can we sync up soon-ish and decide if we can move forward with dropping support for Bundler v1, rather than having potentially incompatible support?
If we do decide we want to move forward with this change, we should IMO at the very least test the scenario where we're using multiple sources with Bundler 1 and see how this implementation handles that, so we can update our documentation to reflect the limitation if necessary.
PS: @deivid-rodriguez I thoroughly appreciate your work on this, I absolutely see you're trying to move us to a better place, and I can see the benefits of this approach ❤️
No worries @jurre, I understand being careful, and that's precisely why I proposed this as a way to unblock dropping support, since it has been postponed a few times precisely to avoid disrupting users. But I do understand that this will still be disrupting.
How about this alternative approach:
If Dependabot finds a Bundler v1 lockfile, then it ignores all dependencies, except for Bundler itself, and proposes a PR to upgrade Bundler. Once the PR is merged, users can get PRs for the other dependencies again.
Let me talk to Jon about dropping support altogether, and let's also thoroughly test this approach on some real world v1 repo's.
I noticed some v2 fixtures with multiple sources actually use the v1 format, for example this one. I'm actually not quite sure what that means for our test suite here, but either way it'd be good to double check how this actually behaves. @amazimbe is going to run through some examples in a bit.
I was curious and I tried this, and I can confirm the behavior:
- Dependabot will split sources in lockfile for security if used to upgrade a lockfile with merged sources.
- Bundler v1 will either merge them back (if run in non frozen mode), or fail (if run in frozen mode).
Note that this is not a new issue though, it already happened for 2.x lockfiles when Dependabot migrated to the version of Bundler that started splitting sources. See for example: https://github.com/dependabot/dependabot-core/issues/4095.
I think the best course of action would be what I proposed earlier:
- If Dependabot finds a Bundler v1 lockfile, then it ignores all dependencies, except for Bundler itself, and proposes a PR to upgrade Bundler.
- Once that PR is merged, users can get PRs for the other dependencies again.
I run this against a repo with : bundler version 1.17.3 ruby 2.7.0
but it did not update the bundler version.
Yes, that's correct. Currently Dependabot never updates the Bundler version, and that's what will create issues here. Dependabot will upgrade the lockfile to "v2 format" but will keep "BUNDLED WITH 1.17.3" in it. And that will cause issues in edge cases like the multiple sources situation we were discussing.
Hence my suggestion of actually adding support in Dependabot for upgrading the version of Bundler in the lockfile. Then we can completely stop creating PRs for Bundler v1 lockfiles, except for upgrading the Bundler version itself, and once users merge that PR and upgrade to Bundler v2, they get their Dependabot PRs back.
Yes, that's correct. Currently Dependabot never updates the Bundler version, and that's what will create issues here. Dependabot will upgrade the lockfile to "v2 format" but will keep "BUNDLED WITH 1.17.3" in it. And that will cause issues in edge cases like the multiple sources situation we were discussing.
Hence my suggestion of actually adding support in Dependabot for upgrading the version of Bundler in the lockfile. Then we can completely stop creating PRs for Bundler v1 lockfiles, except for upgrading the Bundler version itself, and once users merge that PR and upgrade to Bundler v2, they get their Dependabot PRs back.
In my case the lockfile stays in v1 format
In my case the lockfile stays in v1 format
So, it proposes correct updates and does not update the format? Then that sounds perfect 😅. I have to admit I tried Bundler directly but not Dependabot as in this PR, so maybe you are right and this is fine after all!
Oh, but it seems that repo does not need any updates? Of course if Bundler won't create any PRs it won't change the lockfile format. I think we'd need to test on a repository that actually needs updates. I suggest downgrading some gem manually and committing the updated Gemfile/Gemfile.lock file, and then trying again.
Tagging @jurre and @deivid-rodriguez for final approvals. If there are no more questions, then @amazimbe can get it deployed.
Thanks.
@amazimbe can you share the before and after lockfiles from those updates where we can see the updated dependency but the format staying the same?
I've had a call with @jurre regarding this PR and some of what I wrote earlier has turns out to be incorrect. To avoid confusion, I'll summarise the current state of play in this comment.
Before running this CLI against my bundler v1 repo the Gemfile.lock was like this:
GEM
remote: https://rubygems.org/
remote: https://xxxx:[email protected]/xxxx/
specs:
connection_pool (2.4.1)
hola (0.0.0)
io-console (0.7.2)
irb (1.14.0)
rdoc (>= 4.0.0)
reline (>= 0.4.2)
psych (5.1.2)
stringio
rack (3.1.6)
rack-protection (3.0.6)
rack
rdoc (6.7.0)
psych (>= 4.0.0)
redis (5.2.0)
redis-client (>= 0.22.0)
redis-client (0.22.2)
connection_pool
reline (0.5.9)
io-console (~> 0.5)
sidekiq (6.0.1)
connection_pool (>= 2.2.2)
rack (>= 2.0.0)
rack-protection (>= 2.0.0)
redis (>= 4.1.0)
stringio (3.1.1)
PLATFORMS
ruby
DEPENDENCIES
hola (= 0.0.0)!
irb
sidekiq (= 6.0.1)
BUNDLED WITH
1.17.3
After running this CLI against my bundler v1 repo, the output contained:
GEM
remote: https://rubygems.pkg.github.com/xxxx/
specs:
hola (0.0.0)
GEM
remote: https://rubygems.org/
specs:
concurrent-ruby (1.3.3)
connection_pool (2.4.1)
io-console (0.7.2)
irb (1.14.0)
rdoc (>= 4.0.0)
reline (>= 0.4.2)
logger (1.6.0)
psych (5.1.2)
stringio
rack (3.1.7)
rdoc (6.7.0)
psych (>= 4.0.0)
redis-client (0.22.2)
connection_pool
reline (0.5.9)
io-console (~> 0.5)
sidekiq (7.3.0)
concurrent-ruby (< 2)
connection_pool (>= 2.3.0)
logger
rack (>= 2.2.4)
redis-client (>= 0.22.2)
stringio (3.1.1)
PLATFORMS
ruby
DEPENDENCIES
hola (= 0.0.0)!
irb
sidekiq (= 7.3.0)
BUNDLED WITH
1.17.3
This is indeed in V2 format.
We then run bundle _1.17.3_ update in the v1 repo and the new Gemfile.lock reverted back to the original i.e.
GEM
remote: https://rubygems.org/
remote: https://xxxx:[email protected]/xxxx/
specs:
connection_pool (2.4.1)
hola (0.0.0)
io-console (0.7.2)
irb (1.14.0)
rdoc (>= 4.0.0)
reline (>= 0.4.2)
psych (5.1.2)
stringio
rack (3.1.7)
rack-protection (3.0.6)
rack
rdoc (6.7.0)
psych (>= 4.0.0)
redis (5.2.0)
redis-client (>= 0.22.0)
redis-client (0.22.2)
connection_pool
reline (0.5.9)
io-console (~> 0.5)
sidekiq (6.0.1)
connection_pool (>= 2.2.2)
rack (>= 2.0.0)
rack-protection (>= 2.0.0)
redis (>= 4.1.0)
stringio (3.1.1)
PLATFORMS
ruby
unknown
DEPENDENCIES
hola (= 0.0.0)!
irb
sidekiq (= 6.0.1)
BUNDLED WITH
1.17.3
To conclude, I believe we have 3 main options:
- Remove support for bundler v1 completely as bundler v1 is more than 6 years old. But as @deivid-rodriguez mentioned earlier, this has been tried before but we haven't actually done it due to the number of v1 users still being significant.
- As proposed by @deivid-rodriguez, make changes to Dependabot so that it only creates a PR to upgrade bundler if it encounters a bundler v1 lockfile. This would be like a forced bundler upgrade.
- Merge this as is and document the flip flopping when users manually run
bundle updateto avoid confusion. From what I've seen, this would lose the updates made by dependabot every timebundle updateis run so it could be confusing and perhaps ineffective.