Github actions are not used when determining PR status

[lindell]

Github actions are not used when determining PR status

[lindell]

From your second comment, it seems that the changes detect failing GitHub actions as expected, which was the intended change, so that is good. Even though you have the additional use case of ignoring it. Could I get an explicit confirmation of this? 🙂

---

I think that multi-gitter should have the option to respect the number of approvers settings. Maybe with the name you suggested in the original comment --require-approvals. But I don't think that should be the default behaviour since most users want to make the changes without any manual labor. Could you maybe create a separate issue specifically for that? So that any discussions are isolated to that specific topic. I do intend to close this issue if the problem of GitHub actions not working as other CIs becomes solved.

---

I have a hard time seeing the use case of pushing code that is not working. And not setting an explicit requirement that code can never be merged that isn't passing CI should not change the behavior of multi-gitter to always merge broken code. But if you have a requirement to ignore this, please create a separate issue for this as well so that it could be discussed separately.

[lindell]

I don't know which SCM you are using, but assuming it's Github. This should already be implemented (for status checks, not necessarily approvals).

The status is set here based on status checks:
https://github.com/lindell/multi-gitter/blob/master/internal/scm/github/github.go#L577

And only successful PRs are merged:
https://github.com/lindell/multi-gitter/blob/master/internal/multigitter/merge.go#L26

[ryancurrah]

That should be implemented by the CI software and prevent merging if those checks are not satisfied.

[dave-v]

I am using GitHub. I've created a test repo QuickRelease/test-multi-gitter-merge. It has a status check that always fails, and branch protection requiring that the fail status check passes as well as an approving review.

Here is a PR (QuickRelease/test-multi-gitter-merge#1) I just merged using multi-gitter merge --repo QuickRelease/test-multi-gitter-merge -B something, and it successfully merged, even though it should have been blocked.

This seems to be because my account privileges allow me to override the branch protection and merge anyway. When I tick the "Include administrators" box, merging does indeed fail. In my general use-case, this box tends not be ticked, so I personally would find it useful to be able to still enforce those checks have passed despite being an administrator on a repo.

(Also, before I merged the PR, multi-gitter status --repo QuickRelease/test-multi-gitter-merge -B something returned a status of Success, which seems strange - does this just refer to the fact that a PR was successfully raised? I assumed it would show the status of the PR itself.)

[lindell]

I assumed it would show the status of the PR itself.)

It does show if the CI pipelines was succeeded or not.
I've used this myself in my previous jobs and it did correctly reflect the status of the Travis checks, even if I had admin privileges.

Maybe GitHub actions is treated differently? But that seems very strange. I will try to take a look.

[lindell]

This seems to be an issue with the status endpoint reporting everything except GitHub actions!

Others projects seem to have done the exact same assumption:
microsoft/vscode-pull-request-github#1105

The solution will have to be to manually fetch these checks and include that is the logic to determine the status.

[lindell]

It seems that the only good way to gather this info was to pull the information from the GraphQL API. I took the initiative to make sure that the entire pull request part pulls the data through it, so that it becomes much faster.

#242
Please try it out by installing it with: go install github.com/lindell/multi-gitter@graphql-prs

Maybe @wstrm or @Mseller could try it on a Github Enterprise instance to confirm that it works as expected there as well :)

[Nyxonios]

Will do! 👍

[dave-v]

Thanks for looking into this issue. I have tried it out with my test repo. I found two issues. I think the first is more important than the second.

---

With the following setup...

Branch protection rule (everything else unticked):

Which in the GitHub UI looks like this:

I would expect the merge to fail.

multi-gitter merge will in fact merge this PR.

---

With the following setup...

Branch protection rule (i.e. we do not care if status checks pass):

Which in the GitHub UI looks like this:

I would expect the merge to work.

multi-gitter merge will fail to merge.

[lindell]

Any update on how it went @Mseller? 🙂

[Nyxonios]

Sorry, been very busy with other stuff, will try to get this done next week! 😄

[wstrm]

From your second comment, it seems that the changes detect failing GitHub actions as expected, which was the intended change, so that is good. Even though you have the additional use case of ignoring it. Could I get an explicit confirmation of this? 🙂

Because the "Branch protection rule" is not enabled the "Merge pull request" button is not disabled -- and therefore allowed to merge. I would expect that it would merge in this case because the repository is configured to not enforce status checks? :)

[lindell]

The problem is that the enforcement is turned off by default on GitHub. This means that for most scenarios, when a developer wants to use the merge command, they would merge both the successful and the errored PRs.

"Just because you can doesn't mean you should." 😄

But even if the setting was on by default, I think it would be a bad idea. If someone would have turned it off, it does not mean they want to merge erroneous PRs, they just want to be blocked to do so in rare circumstances when needed. The changes you run through MG could ofc be that rare circumstance, but I think it would be worth specifying that with a flag/doing it manually to avoid a lot more developers merging failing changes.

[wstrm]

Hm, yes you won me over, it sounds sane to not allow the merge to happen :)

[wstrm]

I tested it out on GHE and it seems to work? :)

Installed with:

ξ go install github.com/lindell/multi-gitter@graphql-prs

Pending CI in (GitHub Actions):

ξ multi-gitter status -g https://<redacted> -R <redacted>/test-multi-gitter
<redacted>/test-multi-gitter #1: Pending

ξ multi-gitter merge -g https://<redacted> -R <redacted>/test-multi-gitter
INFO[0000] Merging 0 pull requests

Errored CI:

ξ multi-gitter status -g https://<redacted> -R <redacted>/test-multi-gitter
<redacted>/test-multi-gitter #1: Error

ξ multi-gitter merge -g https://<redacted> -R <redacted>/test-multi-gitter 
INFO[0000] Merging 0 pull requests

Successful CI:

ξ multi-gitter status -g https://<redacted> -R <redacted>/test-multi-gitter
<redacted>/test-multi-gitter #1: Success

ξ multi-gitter merge -g https://<redacted> -R <redacted>/test-multi-gitter 
INFO[0000] Merging 1 pull requests                      
INFO[0000] Merging                                       pr="<redacted>/test-multi-gitter #1"

[lindell]

Thanks for confirming @wstrm 👍 The PR is now merged.

I will now close this PR, but please create new ones for any of the other suggestions posted here so that they can be discussed separately.