Mergifyio / mergify

Mergify Community Issue Tracker
https://mergify.com
Apache License 2.0
320 stars 92 forks source link

Branch protection setting 'strict' conflicts with Mergify configuration #722

Open rix0rrr opened 4 years ago

rix0rrr commented 4 years ago

We use Dependabot and Mergify in the same repository.

Dependabot will automatically keep PRs up-to-date with master, unless some other actor has meddled with the PR in the mean time. So if Mergify tries to merge from master, Dependabot stops keeping the PR up to date.

Dependabot can do a better job of keeping the PR up to date, since it can deal with conflicts in package-lock.json etc, so we'd like to keep this job for Dependabot and disable Mergify's "keeping the PR up-to-date behavior."

Unfortunately, just setting strict: false won't work, because Mergify will complain with the above error message:

Branch protection setting 'strict' conflicts with Mergify configuration

Which is correct: our GitHub branch protection setting is strict, but we want Mergify to accept that and merge the branch whenever all branch protection settings are satisfied, without trying to keep the branch up-to-date itself.

If we're going about this the wrong way, I'd like to hear the recommended configuration of Mergify in conjunction with Dependabot.

(*) It is not an option for us to rely on Dependabot's own merge behavior as that requires a company-wide switch that any org member can toggle, which we cannot rely on in an org of ~1000s of people.

jd commented 4 years ago

That sounds like a proper use case indeed and something that is not covered. We'll look into that. Maybe something like having a passive strict mode where Mergify does not do anything rather than failing.

rix0rrr commented 4 years ago

Can you succeed silently instead of failing? I don't know a 100% for sure but I have a suspicion that Mergify currently failing is causing Dependabot to ignore the PR.

rix0rrr commented 4 years ago

Here's an example of a PR that Dependabot/Mergify aren't playing nice together: https://github.com/aws/aws-cdk/pull/6337

bootstraponline commented 4 years ago

I'm seeing the same issue on Flank with mergify.

diba1013 commented 4 years ago

I have been experimenting with mergify and dependabot in the recent days. My initial approach was that the two bots would talk to each other (see an example PR here).

The Idea

A quick background introduction:

  1. PRs must be up to date before merging, this must be done via rebases.
  2. PRs must have at least one approval. Since I want mergify to automatically merge the PR, it shall approve right before it merges (within the same rule).
  3. Build steps must successfully pass.

The idea was that mergify will not touch the PR itself (to do rebases), but instead command dependabot to perform all necessary actions. However, mergify should be in charge of the actual merge, since it will validate the branch protection settings.

My (failed) Attempts

I will outline my attempts to have them talk to each other. Note, that each following attempt replaces the previous attempt:

  1. Force dependabot to rebase every time, which resulted in dependabot complaining.
  2. Have mergify rebase if there are no conflicts (since this will only be done if necessary). To circumvent dependabot, mergify will force dependabot to recreate if there are conflicts. This resulted in dependabot complaining.
  3. Mergify will not touch the PR, only command dependabot if there are conflicts or mergify was sure that it could merge by itself. Merging was redone with strict: smart in an attempt to only look at one PR and give dependabot enough time to update the other PRs on its own. This is the current implementation, but is not fool-proof, mergify will enqueue all PRs at one time and it is a race condition on who is the fastest. As seen here, mergify will still touch the PR and conflicts may arise.

My Proposal

  1. Maybe add an outdated attribute which will check, if the pull request is up to date with the base branch. This would allow mergify to respond properly to the state of the PR (similar to how GitHub displays the branch protection status). This would enable us to mimic the branch protection rules more closely, if needed, to copy them, and even to move all actions (rebase and merge) to dependabot.
  2. I will not want mergify to touch the PR, which is what strict requires currently (either via merge, rebase or squash). However, I cannot rely on dependabot to always update its PRs in real-time, because it gets stuck some times. Thus mergify should bug dependabot to keep its PR up to date (via command). As remarked above, this could have something to do with mergify failing and dependabot ignoring that PR, as to not update it.

I would envision a configuration like the below. For a comparison, please refer to my current configuration. Disclaimer: I have no idea if that would work:

pull_request_rules:
  - name: 'rebase bot' # This rule is only required, if dependabot gets stuck. If above is true, this could be removed
    conditions:
      - 'author~=^dependabot(|-preview)\[bot\]$'
      - 'outdated' # NEW!
    actions:
      comment:
        message: '@{{author}} rebase'
      dismiss_reviews:
        approved: Mergifyio

  - name: 'merge bot option 1' # Have dependabot do all the work, but ensure only one at a time
    conditions:
      - 'author~=^dependabot(|-preview)\[bot\]$'
      - '-outdated' # NEW!
    actions:
      review:
        type: APPROVE
        strict: smart # NEW! Only allow one PR at a time to be merged
        message: '@{{author}} merge' # I do not know, if dependabot will complain because mergify has no push access, this might need an author (see below).
        bot_account: diba1013 # NEW! I have no idea, if something like this is possible 'mergify commented on behalf of diba1013' and dependabot will approve of my push rights

  - name: 'merge bot option 2' # Have mergify do all the work, but ensure only one at a time
    conditions:
      - 'author~=^dependabot(|-preview)\[bot\]$'
      - '-outdated' # NEW!
    actions:
      review: {}
      merge: 
        method: merge
        strict: smart
        strict_method: passive # NEW!

Warning: At time of writing, the configuration above is NOT valid. Do not use it inside your mergify configuration

Lastly, I want to thank you for all the great work you guys are doing at mergify!

jd commented 4 years ago

@diba1013 I'm not sure I understand what is wrong with using only strict: smart and not trying to command dependabot in any way?

That's what we've been doing on this repository for ever and it works fine.

diba1013 commented 4 years ago

I'm not sure I understand what is wrong with using only strict: smart and not trying to command dependabot in any way?

Even though I have setup dependabot to check for updates daily, it will often create 3+ PRs for one repository (npm), which will pile up if done for multiple repositories. Because dependencies are mostly not an issue, I would want mergify to merge them automatically to keep workload of me. Strictly speaking, I do not need daily updates, but if it is possible to automate, why not? The alternative would be to reduce the update cycle and manually approve a bunch of PRs at once, like I have done in the past.

When having the strict conditions, that branches must be up to date with the base branch, two of these PRs must be rebased. Ideally, dependabot will do this, because it resolves potential conflicts within the files touched. Having mergify rebase the PR and potentially fail due to the branch protection settings—as @rix0rrr suspects—will not have dependabot update this PR, which then, I need to command.

The first step to automatism is to emulate the current workflow and replace my manual work with what mergify is capable. Thus I tried to outline my thought process above by explaining my attempts I took for this task. If this is possible and even eliminating steps out of the workflow, even better. But I found that, at least when manually approving the dependabot PRs, I often needed to command dependabot to rebase it after several hours, because it did not pick up on changes in the base branch, again, potentially due to mergify failing.

I will try your approach, have mergify ignore dependabot rebase conflicts and will report back in a few days (hopefully with some PRs to show the initially described issue). However, I think that your suggestion with some kind of passive mode will be needed to solve this issue.

diba1013 commented 4 years ago

I have had this running for a couple of days now, with mergify ignoring dependabot. Almost all PRs were merged automatically and I had only a couple that needed manual attendance by having dependabot recreate the PR (because mergify force pushed after a rebase) and that ended up in a merge conflict.

However, this seems to be more managable than having mergify command dependabot. Thus I would appreciate if the above solution was implemented, but I can live without it. Thanks for providing your insights.