feat: add support for github merge queue#6435
Conversation
| } | ||
|
|
||
| func setup(t *testing.T) (events_controllers.VCSEventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParserValidator, *mocks.MockAzureDevopsRequestValidator, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClient, *emocks.MockCommentParsing) { | ||
| func setup(t *testing.T) (events_controllers.VCSEventsController, *mocks.MockGithubRequestValidator, *mocks.MockGitlabRequestParserValidator, *mocks.MockAzureDevopsRequestValidator, *emocks.MockEventParsing, *emocks.MockCommandRunner, *emocks.MockPullCleaner, *vcsmocks.MockClient, *emocks.MockCommentParsing, *emocks.MockCommitStatusUpdater) { |
There was a problem hiding this comment.
not a fan of this function signature - considered a refactor, but it was out of scope of this PR
Signed-off-by: Miguel Varela Ramos <miguelvramos92@gmail.com>
93d8c20 to
f5e861b
Compare
There was a problem hiding this comment.
Pull request overview
Adds opt-in support for GitHub Merge Queue (merge_group) webhook events so Atlantis can unblock the queue by posting successful commit statuses on the merge candidate SHA.
Changes:
- Introduces
--gh-merge-queue-enabled(defaultfalse) /gh-merge-queue-enabledconfig to gatemerge_grouphandling. - Adds
merge_groupwebhook handling forchecks_requestedto post success statuses forplan,apply, andpolicy_checkonmerge_group.head_sha. - Adds unit tests covering enabled/disabled behavior, allowlist enforcement, and ignored actions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
server/user_config.go |
Adds GithubMergeQueueEnabled to user config (mapstructure binding). |
cmd/server.go |
Defines --gh-merge-queue-enabled flag and wires it into boolean flags. |
server/server.go |
Plumbs the flag and CommitStatusUpdater into the events controller. |
server/controllers/events/events_controller.go |
Handles GitHub merge_group events and updates commit statuses on the merge candidate SHA. |
server/controllers/events/events_controller_test.go |
Extends controller test setup and adds new merge queue unit tests. |
Signed-off-by: Miguel Varela Ramos <miguelvramos92@gmail.com>
Signed-off-by: Miguel Varela Ramos <miguelvramos92@gmail.com>
chenrui333
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The webhook path, docs, and tests are a good start, but I think this needs a couple correctness fixes before merge.
- Please gate the automerge/enqueue path on
--gh-merge-queue-enabled.
Right now MergePull unconditionally calls getPullMergeQueueStatus, and if the base branch reports requiresMergeQueue it calls enablePullAutoMerge. That means Atlantis can enqueue a PR even when the operator has not opted into merge queue handling or subscribed the webhook/GitHub App to merge_group. If the queue requires Atlantis statuses, Atlantis will then ignore the merge_group event and the queue can stall indefinitely. It also adds a GraphQL call to every GitHub automerge attempt for users who have not enabled this feature.
Please plumb the flag into the GitHub merge path and only use the GraphQL auto-merge/queue path when merge queue support is enabled. When it is disabled, keep the existing REST merge behavior so operators get the same visible failure instead of a stuck queue.
- Please handle merge queues enforced by GitHub rulesets.
The current detection only checks baseRef.branchProtectionRule.requiresMergeQueue, which covers classic branch protection. Repositories can also enforce merge queues through GitHub rulesets, where branchProtectionRule may be nil even though the branch requires a merge queue. In that case Atlantis still falls through to the REST merge endpoint and gets the same 405 this PR is trying to avoid.
Please update the detection or fallback so ruleset-backed merge queues take the queue/auto-merge path when the feature is enabled. If GitHub does not expose a clean preflight signal here, catching the REST 405 and returning a clear merge-queue/ruleset error would still be better than the generic failure; ideally this gets regression coverage too.
- Please add a test for the GraphQL status-query failure fallback.
MergePull intentionally logs and falls back to direct merge when getPullMergeQueueStatus fails. That is a realistic path for missing token scopes or transient API failures, so it would be good to lock in the expected behavior with a unit test.
Smaller follow-ups:
HandleGithubMergeGroupEventconstructs a partialmodels.PullRequestonly to callUpdateCombined. That works today because status posting usesHeadCommitandBaseRepo, but a short comment or a narrower status-posting helper would make the dependency explicit.enablePullAutoMergesilently defaults unknown merge methods tomerge; logging a warning would make bad config easier to diagnose.- Please double-check that the
v0.43.0+docs badge matches the intended release target.
|
@chenrui333 thanks for the thorough review - I'll address your comments |
Closes #5603
what
why
handling, those statuses are never posted on the merge group SHA and the queue stalls indefinitely.
tests