search

dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.72k stars 1.02k forks source link

Add option to always rebase PR's when out of date with target branch #2224

Open dstufft opened 6 years ago

dstufft commented 6 years ago

If you have your Github repository setup to require branches to be up to date before merging, that makes it somewhat harder to use dependabot because you have to ask dependabot to rebase the PR before it is able to be merged.

It would be great if there was a setting that would have dependabot keep the PRs up to date with the master branch so that PRs are always ready to be merged. It might make sense to only use this option in conjunction with a future https://github.com/dependabot/feedback/issues/185 because if you have 5 open PRs and merge one of them, that's 4 PRs that will end up needing to be rebased.

greysteil commented 6 years ago

Interesting. I can see this making sense, but only for repos that are very diligent about merging Dependabot PRs, or where commits are never made directly to master. Otherwise it could cause a lot of additional CI runs, which would be pretty frustrating.

There would also be an impact on the Dependabot infrastructure here - under the hood a rebase is actually a recreation of the PR, which for cases where there are no conflicts is horribly inefficient. It shouldn't be that hard for me to change that, though - I'll take a quick look today.

Hopefully using @dependabot rebase isn't too painful in the meantime.

dstufft commented 6 years ago

Yea its not really a big issue (and it's roughly equivalent to our current solution, which is hitting the update branch button). Which speaking of that, if I hit the "Update Branch" button, which will merge in master, will that break dependabot's conflict resolution since it will see something else made a change to the branch?

greysteil commented 6 years ago

It will - you can use the @dependabot recreate command to get Dependabot to recreate (essentially rebase) the PR if that's happened, though.

mockdeep commented 5 years ago

Opened a new issue, but maybe the issues with CI flooding could be solved by a one-off @dependabot rebase all command rather than having it run every single time.

greysteil commented 5 years ago

@dependabot rebase all wouldn't fix this one, because the PRs need to be merged one-by-one (but thatnks for the suggestion @mockdeep).

matthewtrask commented 5 years ago

Im giving Dependabot a try and I've heard great things. However I noticed when I go to use the reabse feature, it switches to our master branch instead of staying on the dev branch we select the PR to target. Is there a way to prevent this? Reading above it looks like this shouldn't happen.

greysteil commented 5 years ago

@matthewtrask that sounds like a different issue. Can you tag @dependabot in a PR where that has happened so I can take a look?

mockdeep commented 5 years ago

This issue is still relevant.

slawekjaranowski commented 4 years ago

one proposition - instead of writing comment @dependabot rebase it will be easier if dependabot can react on reaction eg 🚀 it will be easier click than write command

gustavovnicius commented 3 years ago

Any updates on this? I started noticing this behavior (not rebasing automatically) after migrating to GitHub's native dependabot. While still using dependabot-preview this behavior was consistent (always rebasing when detecting changes to the target branch).

asbjornu commented 3 years ago

I have the same experience as @gustavovnicius: The non-GitHub-native Dependabot rebased out of date pull requests automatically. After upgrading to GitHub-native Dependabot, the auto-rebasing has stopped working and I have to:

  1. Go into a pull request from Dependabot and write @dependabot rebase.
  2. Wait (usually several minutes) for the checks to become successful.
  3. Manually merge (since auto-merging is no longer possible, see #1973 and #3253) the pull request.
  4. Repeat from step 1 for every other open Dependabot pull request.

This can take hours of manual labour for projects with many dependency updates and/or slow tests.

gustavovnicius commented 3 years ago

I was wondering if this issue is having enough visibility... Maybe if pinging someone, like @feelepxyz would do the trick? (Navigated to the repo insights to see if there was someone to summon in the thread 😅)

lucacome commented 3 years ago

Maybe we need to ping @asciimike? 🙂

asciimike commented 3 years ago

I don't remember the issue off the top of my head but I know there was a discussion of rebase-strategy: always.

I admit what I don't understand about this PR is that I see two cases:

What am I missing here? Is the former case not working that way, or is there more complexity in the latter case (e.g. some file we don't know about is changing that is affecting mergability)?

dstaley commented 3 years ago

if a Dependabot PR doesn't need to be rebased in order to be merged (e.g. other files on the main branch have changed, but the manifest can still be merged), then I don't see a need to rebase on any change

@asciimike The scenario that I'm looking to prevent by using rebase-strategy: always is this:

The repo is using depA@v1 and depB@v1. Dependabot then opens PRs updating depA and depB to v2. The tests then run on each PR, confirming that depA@v2 + depB@v1 and depA@v1 + depB@v2 pass all checks. The user then merges the PR for depA, meaning main is now using depA@v2 + depB@v1. However, since there's no conflict, the PR for depB@v2 is never updated, meaning that at no point has Dependabot verified that depA@v2 + depB@v2 works. The user, not realizing this, then merges the PR for depB, bringing the main branch to depA@v2 + depB@v2, which now fails checks.

One way I've prevented this scenario is to force all PRs to be up-to-date before merging. Dependabot actually can detect this state, and will update PRs so that they're mergeable. However, that branch security rule is only available for paid accounts, so having this functionality built into Dependabot would be nice, especially for personal projects.

lucacome commented 3 years ago

One way I've prevented this scenario is to force all PRs to be up-to-date before merging. Dependabot actually can detect this state, and will update PRs so that they're mergeable.

We have it enabled, but dependabot is not updating those PRs 🤔

asciimike commented 3 years ago

The repo is using depA@v1 and depB@v1. Dependabot then opens PRs updating depA and depB to v2. The tests then run on each PR, confirming that depA@v2 + depB@v1 and depA@v1 + depB@v2 pass all checks. The user then merges the PR for depA, meaning main is now using depA@v2 + depB@v1. However, since there's no conflict, the PR for depB@v2 is never updated, meaning that at no point has Dependabot verified that depA@v2 + depB@v2 works. The user, not realizing this, then merges the PR for depB, bringing the main branch to depA@v2 + depB@v2, which now fails checks.

Aha! This is an excellent description, thank you very much @dstaley!

Two observations:

dstaley commented 3 years ago

Would Add grouped updates #1190 be useful here

In this particular example, I was assuming that depA and depB are independent packages, unrelated to one another, so I don't think there's a logical grouping of them that would make sense.

It feels like Dependabot should rebase any time the manifest file is changed

I know in the past the reason Dependabot dropped this feature was to prevent unwanted/unnecessary CI load. While I totally agree it probably doesn't need to be the default, it would be nice for Dependabot to have an opt-in mode that always rebased PRs. Branch protection rules get us the same experience, but it's an additional burden on maintainers who might not want to be forced to rebase their own PRs since GitHub doesn't allow you to selectively apply branch protection rules based on the author.

deivid-rodriguez commented 1 year ago

I think this is actually the current behavior, and people is requested the exact opposite to this because it's too noisy for many. I'll close this since I believe this is "fixed" but please reopen if I'm wrong!

remal commented 1 year ago

@deivid-rodriguez I suppose the issue is not about changing the current behavior but about adding another option for rebase-strategy setting.

I suppose a setting similar to Renovate's rebaseWhen: behind-base-branch can be a solution. See https://docs.renovatebot.com/configuration-options/#rebasewhen.

deivid-rodriguez commented 1 year ago

Thanks for noticing this @remal. I was rushing too much yesterday and didn't think this through properly. Dependabot currently rebases PRs automatically when dependency manifests change in the target branch, not not whenever a PR becomes out of date with the target branch like this issue is requesting. Reopening.

orange-buffalo commented 1 year ago

Just in case someone is looking for a workaround until this is addressed, I've prepared an action orange-buffalo/dependabot-auto-rebase that requests Dependabot to rebase such PRs on push to main.

elliotwestlake commented 11 months ago

Any movement on this issue? We are moving from renovate and this is a feature we really miss! 😃

roldengarm commented 3 months ago

As a workaround, you could use e.g. https://github.com/marketplace/actions/auto-update to auto update branches. It won't use dependabot to do the rebasing though.

brianespinosa commented 1 month ago

As a workaround, you could use e.g. https://github.com/marketplace/actions/auto-update to auto update branches. It won't use dependabot to do the rebasing though.

That action will not work because it will not trigger new CI runs after it updates. This is detailed in the Limitations section for that action.