cli / cli

GitHub’s official command line tool
https://cli.github.com
MIT License
36.62k stars 5.59k forks source link

gh pr merge --auto should fail if auto-merge is not available. #8792

Open Yaminyam opened 6 months ago

Yaminyam commented 6 months ago

https://github.com/cli/cli/issues/2619 In the above issue, the auto-merge option can now be used in cli. image If you use this command when the auto-merge setting is turned off in the repo, the merge will be performed without checking the required workflow. I think this works differently than intended. If the repo has the auto-merge setting turned off, I think the command should fail when using the --auto option.

williammartin commented 6 months ago

Thanks for the feedback @Yaminyam. Can you tell me a little bit more about your workflow here?

I understand what you're saying but I'm also wondering when this would be a problem and I want to be sure we solve the right thing.

Yaminyam commented 6 months ago

Thank you for quick response! @williammartin As shown in the picture above, I did not turn on the repo's auto-merge setting. I ran the command gh pr merge --auto (this was my mistake) Then, it was merged like a normal merge regardless of the required workflow. I think it's more natural for the gh pr merge --auto command to fail and allows users to catch their mistakes more quickly.

I am attaching the PR I ran as reference material. https://github.com/Yaminyam/backend.ai/pull/70 https://github.com/Yaminyam/backend.ai/actions/runs/8170027326/job/22335314550#step:7:1

williammartin commented 6 months ago

I think it's more natural for the gh pr merge --auto command to fail and allows users to catch their mistakes more quickly.

So I think your fundamental problem here was that you expected your PR to not be merged until all the required checks had passed but it was merged anyway.

What I'm unclear about is why the platform allowed the merge to succeed anyway, if there were still required checks. The only way I'm aware of this happening is if the token being used is for an admin (hence the --admin flag). However, I don't think that tokens in actions are admin.

Can you share a screenshot of your branch protections that would require status checks to pass?

Yaminyam commented 6 months ago

That's right, because I used my PAT in my repo, you probably have admin privileges. And below is my branch protection screenshot.

CleanShot 2024-03-06 at 19 33 36@2x
williammartin commented 6 months ago

Can you show me where the PAT is used in your workflow, I only see ${{ github.token }} which I believe is injected by the actions runners. That said, if you are using a PAT, that probably explains it.

Yaminyam commented 6 months ago

https://github.com/Yaminyam/backend.ai/actions/runs/8170027326/workflow#L104 Oh, sorry. My mistake. There was confusion with the PAT used in the link above. As shown in the link below, it is correct that gh pr merge uses github.token. https://github.com/Yaminyam/backend.ai/actions/runs/8170027326/workflow#L115

Yaminyam commented 6 months ago

I would like to emphasize more that it is awkward for the --auto option to be executed even though auto-merge is disabled than that pr is merged even though the required workflow exists.

williammartin commented 6 months ago

I would like to emphasize more that it is awkward for the --auto option to be executed

Can you share the output of --auto being executed when it shouldn't with GH_DEBUG=api set in the environment?

Yaminyam commented 6 months ago
CleanShot 2024-03-07 at 21 28 06@2x

As you said, I tested with GH_DEBUG=api turned on. Then, the --auto option sometimes failed and sometimes succeeded. (The repo's auo-merge setting is always disabled) My guess is that if you run merge with the --auto option as soon as pr is created, it will be merged.

Yaminyam commented 6 months ago

The reason I expected that was because when I used the cli command at high speed, the command succeeded, and secondly, it always succeeded in automated fast situations such as actions.

williammartin commented 6 months ago

Your screenshot doesn't show running with GH_DEBUG=api turned on. You need to do either:

GH_DEBUG=api gh pr merge ...

or

export GH_DEBUG=api
gh pr merge ...

Running GH_DEBUG=api on its own line has no effect on anything. I would suggest you use the first example otherwise every subsequent command will have this env var set and you'll need to unset GH_DEBUG.

My guess is that if you run merge with the --auto option as soon as pr is created, it will be merged.

Interesting. The debug output might hint at something, so let's make sure to get that.

Yaminyam commented 6 months ago

GH_DEBUG=api gh pr merge 88 --auto --squash I ran the above command twice, and the first time failed as shown in the first log, but when I tried it again, it succeeded. As I expected above, I also confirmed that the difference from the time when the pr was created does not matter much. It just seems like the command itself performs various actions. However, there is still no way to infer why GitHub Actions always succeeds.

[git rev-parse --verify refs/heads/backport-test]
* Request at 2024-03-08 16:08:16.910793 +0900 KST m=+0.498117001
* Request to https://api.github.com/graphql
> POST /graphql HTTP/1.1
> Host: api.github.com
> Accept: application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview
> Authorization: token ████████████████████
> Content-Length: 229
> Content-Type: application/json
> Graphql-Features: merge_queue
> Time-Zone: Asia/Seoul
> User-Agent: GitHub CLI 2.45.0

GraphQL query:
mutation PullRequestAutoMerge($input:EnablePullRequestAutoMergeInput!){enablePullRequestAutoMerge(input: $input){clientMutationId}}
GraphQL variables: {"input":{"pullRequestId":"PR_kwDOIFka0c5pDHBl","mergeMethod":"SQUASH"}}

< HTTP/2.0 200 OK
< Access-Control-Allow-Origin: *
< Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset
< Content-Security-Policy: default-src 'none'
< Content-Type: application/json; charset=utf-8
< Date: Fri, 08 Mar 2024 07:08:17 GMT
< Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin
< Server: GitHub.com
< Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
< Vary: Accept-Encoding, Accept, X-Requested-With
< X-Accepted-Oauth-Scopes: repo
< X-Content-Type-Options: nosniff
< X-Frame-Options: deny
< X-Github-Media-Type: github.merge-info-preview; param=nebula-preview; format=json
< X-Github-Request-Id: E306:1CB7AF:306753:347B91:65EAB960
< X-Oauth-Client-Id: 178c6fc778ccc68e1d6a
< X-Oauth-Scopes: gist, read:org, read:project, repo, workflow
< X-Ratelimit-Limit: 5000
< X-Ratelimit-Remaining: 4986
< X-Ratelimit-Reset: 1709883346
< X-Ratelimit-Resource: graphql
< X-Ratelimit-Used: 14
< X-Xss-Protection: 0

{
  "data": {
    "enablePullRequestAutoMerge": null
  },
  "errors": [
    {
      "type": "UNPROCESSABLE",
      "path": [
        "enablePullRequestAutoMerge"
      ],
      "locations": [
        {
          "line": 1,
          "column": 72
        }
      ],
      "message": "Pull request Auto merge is not allowed for this repository"
    }
  ]
}

* Request took 402.583625ms
GraphQL: Pull request Auto merge is not allowed for this repository (enablePullRequestAutoMerge)
[git rev-parse --verify refs/heads/backport-test]
* Request at 2024-03-08 16:08:47.619934 +0900 KST m=+0.530687876
* Request to https://api.github.com/graphql
> POST /graphql HTTP/1.1
> Host: api.github.com
> Accept: application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview
> Authorization: token ████████████████████
> Content-Length: 205
> Content-Type: application/json
> Graphql-Features: merge_queue
> Time-Zone: Asia/Seoul
> User-Agent: GitHub CLI 2.45.0

GraphQL query:
mutation PullRequestMerge($input:MergePullRequestInput!){mergePullRequest(input: $input){clientMutationId}}
GraphQL variables: {"input":{"pullRequestId":"PR_kwDOIFka0c5pDHBl","mergeMethod":"SQUASH"}}

< HTTP/2.0 200 OK
< Access-Control-Allow-Origin: *
< Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset
< Content-Security-Policy: default-src 'none'
< Content-Type: application/json; charset=utf-8
< Date: Fri, 08 Mar 2024 07:08:49 GMT
< Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin
< Server: GitHub.com
< Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
< Vary: Accept-Encoding, Accept, X-Requested-With
< X-Accepted-Oauth-Scopes: repo
< X-Content-Type-Options: nosniff
< X-Frame-Options: deny
< X-Github-Media-Type: github.merge-info-preview; param=nebula-preview; format=json
< X-Github-Request-Id: E30F:CD666:2E311D:322E04:65EAB97F
< X-Oauth-Client-Id: 178c6fc778ccc68e1d6a
< X-Oauth-Scopes: gist, read:org, read:project, repo, workflow
< X-Ratelimit-Limit: 5000
< X-Ratelimit-Remaining: 4984
< X-Ratelimit-Reset: 1709883346
< X-Ratelimit-Resource: graphql
< X-Ratelimit-Used: 16
< X-Xss-Protection: 0

{
  "data": {
    "mergePullRequest": {
      "clientMutationId": null
    }
  }
}

* Request took 1.5913855s
✓ Squashed and merged pull request #88 (Update backend.ai)
Yaminyam commented 6 months ago
CleanShot 2024-03-08 at 16 35 05@2x

Rather than checking allow auto-merge, I first check whether the required actions have passed, and if so, I suspect that merging is being done. I have registered a test with required actions, but it keeps failing as shown in the picture above. If I wait long enough for the test actions to complete and try again, the merge succeeds.

However, I still don't know why merge always succeeds in actions.

Yaminyam commented 6 months ago

https://github.com/cli/cli/blob/4601d7ff5e8f87be07b7b97aafedc93deeeca3ef/pkg/cmd/pr/merge/http.go#L88 Since auto-merge allow is exception handled by the github backend server and a failure is thrown, enableAutoMerge() API must be called regardless of the pr status, but the cli determines the pr status and forcibly disables the auto option. As a result, it is understood that the general merge() API is called and the merge occurs. https://github.com/cli/cli/blob/4601d7ff5e8f87be07b7b97aafedc93deeeca3ef/pkg/cmd/pr/merge/merge.go#L510C47-L510C92 Looking at the code above, the auto value of mergePayload is being checked once again with the !isImmediatelyMergeable(pr.MergeStateStatus) function, and the auto option is being disabled in this process.

Yaminyam commented 6 months ago

https://github.com/Yaminyam/backend.ai/actions/runs/8200185701/job/22426486616#step:8:552 Since mergeStateStatus is UNSTABLE, it seems that the merge will be successful. When does mergeStateStatus become UNSTABLE?

https://docs.github.com/en/enterprise-cloud@latest/graphql/reference/enums#mergestatestatus I don't understand what non-passing commit means.

Yaminyam commented 6 months ago

https://github.com/cli/cli/commit/6f2dfd7eea1e4cfc004417a778ce2058c814d22a If you look at the commit in question, it is intended to perform a normal merge when it is clean or has_hook. If it is in a clean state, it says that a normal merge is performed immediately without using auto-merge. https://github.com/cli/cli/pull/3706

As mentioned above, I think it is natural that the --auto option itself will fail if the repo's auto-merge option is turned off. Therefore, I think the natural behavior is to remove the !isImmediatelyMergeable(pr.MergeStateStatus) code and always call enableAutoMerge(). I'm trying to modify the code like this. What do you think? @williammartin

Yaminyam commented 6 months ago

https://github.com/cli/cli/pull/3706/commits/6e026412dfb0e719584d106bfad0cb1df547cb20 In that commit, a normal merge was performed even in the UNSTABLE state, but I don't know why.