Missing CODEOWNERS fails to merge PR ungracefully

[howardjohn]

trafficstars

On a PR that had been approved, but not by codeowners we get the error message:

Branch protection settings are blocking automatic merging See: https://doc.mergify.io/faq.html#mergify-is-unable-to-merge-my-pull-request-due-to-my-branch-protection-settings GitHub error message: Waiting on code owner review from ...

This is pretty misleading because it looks like mergify is broken, but it is actually expected that it isn't merged.

Is it possible for mergify to expect codeowners approval?

Note - this is pretty much a cosmetic issue only

[nomeata]

Hmm, this is more than cosmetic, isn't it? If mergify believes it can merge (but Github disagrees due to lack of code owner approval), and strict merges are enabled, mergify will prematurely refresh the branch. (Possibly repeatedly maybe even, I saw that in the past, not sure if that is still a problem.)

[jd]

If branch protection is enabled, then Mergify knows the PR is not mergeable so it does not update it.

[nomeata]

Indeed, I was confused by some unrelated hickup. sorry for the noise.

[nomeata]

Actually, no, I think I still see this: The mergify conditions are satisfied, but not all code owners have approved, and now mergify repeatedly updates the branch.

[nomeata]

Maybe there should simply “a can be merged” attribute in https://doc.mergify.io/conditions.html#attributes?

Our setup currently has

      - "#approved-reviews-by>=1"
      - "#changes-requested-reviews-by=0"
      - status-success=our-check

but really that is just an approximation for the rules that GitHub has already for the branch (must be up-to-date, must have at least one approval, all code owners must have approved, nobody must have requested changes, CI must pass). So wouldn’t it be easiest if I would not have to repeat the rules (incompletely) to mergify?

[jd]

This ought to be the default actually. It's unclear to me why CODEOWNERS would not be honored by GitHub API when returning the status of the PR merge-ability.

[nomeata]

Do you use GitHub's mergability to decide whether to update the branch in order to prepare for a strict merge? (I guess not, because if strict merges are required, the PR will be considered unmergeable)

[jd]

We can't use GitHub mergeability state for this because it's bugged. We reached GitHub support multiple times on this, but they failed to fix it. The API provides a correct value 95% of the time, but in some cases, it is not outdated and shows something different than what the user sees in the UI (!).

That's why the engine ignores the field and tries to merge if the conditions are valid. It might try the merge (and therefore updating/rebasing the PR) even if it's obvious for a human that the CODEOWNER file is going to block the merger.

Since GitHub is not able to provide a correct field for the engine, that means it'd need to implement all the branch protection features itself with the same logic to compute the mergeability state itself. It does not do that yet, mainly because it's a huge piece of work to do and to get right, and also because we so far hoped for GitHub to fix the issue.

[basvandijk]

Thanks for the update Julien!

Since any CODEOWNER (person or team) is automatically requested for review by GitHub we're considering adding the Mergify condition:

      - "#approved-reviews-by=#review-requested"

to approximate the CODEOWNERS logic.

[nomeata]

I woner if review-requested means “requested, but not actually done (or dismissed)” or “requested, and possibly approved”. @jd, can you clarify (ideally in the docs). In the former case, we might want

      - "#approved-reviews-by>=0"
      - "#review-requested=0"

[nomeata]

Also, our internal testing shows that mergeable_state = behind, independent of whether there are the right reviews or not, which kinda matches the docs in https://developer.github.com/v4/enum/mergestatestatus/. How do you hope to read “will be mergable after being updated” from the API, or are you referring to some other Github API? (My pessimism that the information we want simply isn't there, even if it were not buggy, didn’t die yet)

[basvandijk]

I'm getting a The new Mergify configuration is invalid when adding the #approved-reviews-by=#review-requested condition. So I guess that's not supported by Mergify.

[jd]

      - "#approved-reviews-by=#review-requested"

Would be a good idea but you can't use variables in the right part of an expression. :(

review-requested contains the list of reviews requested by not fulfilled so I think @nomeata suggestion is good workaround, though I'd use >0 to be sure you have at least one approval.

 - "#approved-reviews-by>0"
 - "#review-requested=0"

The problem with mergeable_state is that it can be set to blocked while it's not. It's a nasty bug because the UI on github.com will get the right state and show you a green merge button, whereas Mergify won't do anything because it'd see the wrong blocked state…

This is noted here if you're curious: https://github.com/Mergifyio/mergify-engine/blob/master/mergify_engine/actions/merge/helpers.py#L52-L58

[nomeata]

The problem with mergeable_state is that it can be set to blocked while it's not.

Hmm, I don’t think that’s what I mean. I expect that we get mergable_state=behind, if the PR is behind master, no matter if it has CODEOWNER approval or not. So therefore the logic “update if behind and (otherwise) mergable” is not implementable, as you can’t distinguish it from `“bedind and missing reviews”.

[jd]

@nomeata oh yeah, definitely. That's another issue indeed.

[nomeata]

Is there any hope in a fix for this? Maybe reimplement CODEOWNERS logic externally? This is constantly adding friction here…

[jd]

Not for now. We're tracking this on our roadmap. I think it'd be great to have an alternative to CODEOWNERS entirely so branch protection could be disabled.

Do you have any idea of what would be the perfect workflow for you with Mergify if you had to replace CODEOWNERS, @nomeata ?

[Borda]

is this CODEOWNERS resolved, I found that some other bots have explicitly set how many code-owners has to approve

[jd]

It's solved for branch protections, as in Mergify now move back the PR at the end of the queue if it can't be merged because of code owners not matching. So it works, it's not optimized as Mergify can't get the protection status to know if it's worth putting the PR into the queue or not.