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

Dependabot bumped a subdependency from 8.x to 9.0 when updating a gem #2246

Open
connorshea opened this issue Jul 23, 2018 · 12 comments
Open
Labels
F: language-support Issues specific to a particular language or ecosystem; may be paired with an L: label. F: version-updates ⬆️ Issues specific to version updates L: ruby:bundler RubyGems via bundler T: feature-request Requests for new features

Comments

@connorshea
Copy link

Dependabot updated bootstrap from 4.1.1 to 4.1.2, but in the process also updated autoprefixer-rails from 8.6.5 to 9.0.0.

connorshea/mdn-compat-data-explorer#117

I think this just might be a limitation of bundler (IIRC it updates the subdependencies of any gems that are updated, not sure if there's a flag you can pass to stop this?). But I feel like this isn't the ideal behavior, it should at least block the bootstrap PR from being merged since a dependency had a major update.

@connorshea
Copy link
Author

This also, presumably, has the side effect of poisoning the compatibility score for this bootstrap version update. Since this autoprefixer-rails update had a 72% pass rate, this bootstrap upgrade had a lower score than it otherwise would have, an 86%.

@greysteil
Copy link
Contributor

Interesting. I think Dependabot / Bundler is doing the right thing here, but it's not totally uncontroversial.

Basically, Dependabot is running the equivalent of a normal bundle update bootstrap here, which means all of bootstraps sub-dependencies which aren't listed in the Gemfile get unlocked and will update if they can. The idea (on Bundler's side, as well as Dependabot's) is that bumping the major version of a sub-dependency should be no big deal, which is true if all dependency authors write correct version ranges for their dependencies.

The conservative part of me wants to change that, and tell Dependabot to only update sub-dependencies if it has to (which is an option in Bundler, although I don't have total confidence in it as I'm not sure how it interacts with the resolver). However, at the moment Dependabot doesn't create any PRs that explicitly target sub-dependencies, so doing so would mean they could really lag behind. Doing the optimistic update when updating a dependency that uses them is an imperfect compromise.

In future, I think the right approach for Dependabot might be to have an additional option to create PRs for sub-dependencies (we can actually already do it, and a couple of people have it enabled). If a user enables that then Dependabot should also run updates for top-level dependencies in conservative mode.

@connorshea
Copy link
Author

I have the separate subdependency updates enabled which is why I noticed this in the first place :D

@greysteil
Copy link
Contributor

Haha, goes to show how little trouble it's caused on my side that I'd forgotten that! How's it working out for you?

@connorshea
Copy link
Author

Quite well, no problems that I've noticed so far. It might cause too many PRs if I had a larger project, but for my use case it's great.

@connorshea
Copy link
Author

Also, I think it should be emphasized that this behavior makes the compatibility score less relevant. It means that bootstrap has a patch update with very minimal changes that also bring with it a major version bump for autoprefixer-rails. This causes failures to be attributed to bootstrap when they're actually problems with autoprefixer-rails.

I don't know what the correct solution is here, but I think that's the most important problem this causes.

That, and maybe the fact that it would cause major dependency updates to happen under the user's nose.

@connorshea
Copy link
Author

At the least, I think the PR should mention that autoprefixer-rails was also updated. For example, the PR title could be changed from Bump bootstrap from 4.1.1 to 4.1.2 to Bump bootstrap from 4.1.1 to 4.1.2 (+1 sub-dependency). It'd also be nice to have autoprefixer-rails' changelog in the PR description.

(also, apologies for the number of comments! I keep thinking of things I want to add 😄)

@shimbaco
Copy link

I wish Dependabot will run bundle update bootstrap --conservative not bundle update bootstrap.
https://bundler.io/man/bundle-update.1.html

Thanks!

@maschwenk
Copy link

While I think we should allow semver to do its job, because dependabot doesn't include the changelog of the sub-dependencies, I actually have to go reach out and see if the sub-dependencies have any sneaky breaking changes.

All it would take is some configuration option for --conservative, which I use all the time on my own projects.

@pwim
Copy link

pwim commented May 26, 2019

I don't mind if subdependencies are updated, but this should be made explicit in the PR, ideally with listing the relevant changelog etc.

@greysteil
Copy link
Contributor

Thanks for the feedback. As you can imagine, we're a little snowed under with requests right now!

The reason Dependabot doesn't use --conservative is to ensure sub-dependencies are still regularly updated, as by default it won't open PRs for them directly. For folks who have enabled sub-dependency updates I think --conservative would be better, though, and we can look at making that configurable.

On changelogs for sub-dependencies, we don't currently pass that information out of dependabot-core, so it's a non-trivial change. I agree it would be an improvement, though.

@infin8x infin8x transferred this issue from dependabot/feedback Jun 29, 2020
@infin8x infin8x added F: language-support Issues specific to a particular language or ecosystem; may be paired with an L: label. L: ruby:bundler RubyGems via bundler T: feature-request Requests for new features labels Jul 2, 2020
@deivid-rodriguez deivid-rodriguez added the F: version-updates ⬆️ Issues specific to version updates label Aug 31, 2022
@dependabot dependabot deleted a comment from stale bot Feb 6, 2023
@rusterholz
Copy link

@greysteil Any progress on making --conservative a configurable option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: language-support Issues specific to a particular language or ecosystem; may be paired with an L: label. F: version-updates ⬆️ Issues specific to version updates L: ruby:bundler RubyGems via bundler T: feature-request Requests for new features
Projects
None yet
Development

No branches or pull requests

8 participants