--gh-allow-mergeable-bypass-apply skips status checks #2624

jffrose · 2022-10-27T18:31:24Z

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
If you are interested in working on this issue or have submitted a pull request, please leave a comment.

Overview of the Issue

When testing out the -gh-allow-mergeable-bypass-apply option on the Atlantis server I noticed that this action allows Atlantis to skip additional mergeable checks that are not part of a protected branch's status checks.

For example, a Protected branch can prevent merges on Require conversation resolution before merging. If this box is checked for a branch and the following workflow happens then atlantis apply still passes.

Reproduction Steps

enable Require conversation resolution before merging on protected branch
Run Atlantis with -gh-allow-mergeable-bypass-apply
Require apply_requirement:[mergeable]
Create PR
Create unresolved comment on PR
Run atlantis apply
Observe that apply succeeds

If I don't use the -gh-allow-mergeable-bypass-apply flag then the same PR's atlantis apply fails.

Logs

``` {"level":"debug","ts":"2022-10-27T14:48:33.436-0400","caller":"server/middleware.go:45","msg":"POST /events – from [::1]:65255","json":{}} {"level":"debug","ts":"2022-10-27T14:48:33.436-0400","caller":"events/events_controller.go:98","msg":"handling GitHub post","json":{}} {"level":"debug","ts":"2022-10-27T14:48:33.456-0400","caller":"events/events_controller.go:163","msg":"request valid","json":{"gh-request-id":"X-Github-Delivery=f4948200-5627-11ed-9925-aae3d0ca0550"}} {"level":"info","ts":"2022-10-27T14:48:33.457-0400","caller":"events/events_controller.go:533","msg":"parsed comment as command=\"apply\" verbose=false dir=\"\" workspace=\"\" project=\"\" flags=\"\"","json":{"gh-request-id":"X-Github-Delivery=f4948200-5627-11ed-9925-aae3d0ca0550"}} {"level":"debug","ts":"2022-10-27T14:48:33.457-0400","caller":"events/events_controller.go:563","msg":"executing command","json":{"gh-request-id":"X-Github-Delivery=f4948200-5627-11ed-9925-aae3d0ca0550"}} {"level":"debug","ts":"2022-10-27T14:48:33.457-0400","caller":"server/middleware.go:72","msg":"POST /events – respond HTTP 200","json":{}} {"level":"debug","ts":"2022-10-27T14:48:33.764-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.get_pull_request.execution_time","value":0.306741833,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:34.127-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.get_pull_request.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:34.127-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github_event.comment_created.success_200","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:34.181-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.update_status.execution_time","value":0.416329709,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:34.181-0400","caller":"vcs/github_client.go:254","msg":"GET /repos/chronosphereio/atlantis_test/pulls/5/reviews","json":{}} {"level":"debug","ts":"2022-10-27T14:48:34.397-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.pull_is_approved.execution_time","value":0.2156325,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:34.694-0400","caller":"vcs/github_client.go:395","msg":"AllowMergeableBypassApply feature flag is enabled - attempting to bypass apply from mergeable requirements","json":{}} {"name":"atlantis.github.update_status.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:35.127-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.pull_is_approved.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:35.215-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.pull_is_mergeable.execution_time","value":0.818263292,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:35.231-0400","caller":"events/project_command_builder.go:598","msg":"Merging config for project at dir: \"terraform\" workspace: \"default\"","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.231-0400","caller":"valid/global_cfg.go:247","msg":"MergeProjectCfg started","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.231-0400","caller":"valid/global_cfg.go:488","msg":"setting apply_requirements: [] from default server config","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.231-0400","caller":"valid/global_cfg.go:488","msg":"setting workflow: \"default\" from default server config","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.231-0400","caller":"valid/global_cfg.go:488","msg":"setting allowed_overrides: [apply_requirements] from repos[1], id: /.*/","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"valid/global_cfg.go:488","msg":"setting allow_custom_workflows: false from default server config","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"valid/global_cfg.go:488","msg":"setting delete_source_branch_on_merge: false from default server config","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"valid/global_cfg.go:255","msg":"overriding server-defined apply_requirements with repo settings: [mergeable]","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"valid/global_cfg.go:295","msg":"MergeProjectCfg completed","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"valid/global_cfg.go:298","msg":"final settings: apply_requirements: [mergeable], workflow: default","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"events/project_command_context_builder.go:95","msg":"Building project command context for apply","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"info","ts":"2022-10-27T14:48:35.232-0400","caller":"events/project_command_context_builder.go:294","msg":"cannot determine which version to use from terraform configuration, detected 0 possibilities.","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.232-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.cmd.comment.apply.builder.execution_time","value":0.016570459,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:35.550-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.update_status.execution_time","value":0.317586084,"tags":{},"type":"timer"}} {"level":"info","ts":"2022-10-27T14:48:35.550-0400","caller":"runtime/apply_step_runner.go:39","msg":"starting apply","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:35.551-0400","caller":"models/shell_command_runner.go:93","msg":"starting \"/opt/homebrew/bin/terraform apply -input=false \\\"/Users/jeffrose/.atlantis/repos/chronosphereio/atlantis_test/5/default/terraform/default.tfplan\\\"\" in \"/Users/jeffrose/.atlantis/repos/chronosphereio/atlantis_test/5/default/terraform\"","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"info","ts":"2022-10-27T14:48:35.911-0400","caller":"models/shell_command_runner.go:156","msg":"successfully ran \"/opt/homebrew/bin/terraform apply -input=false \\\"/Users/jeffrose/.atlantis/repos/chronosphereio/atlantis_test/5/default/terraform/default.tfplan\\\"\" in \"/Users/jeffrose/.atlantis/repos/chronosphereio/atlantis_test/5/default/terraform\"","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"info","ts":"2022-10-27T14:48:35.912-0400","caller":"runtime/apply_step_runner.go:58","msg":"apply successful, deleting planfile","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:36.126-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.pull_is_mergeable.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:36.126-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.cmd.comment.apply.builder.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:36.126-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.update_status.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:36.126-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.projects","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:36.242-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.update_status.execution_time","value":0.32952325,"tags":{},"type":"timer"}} {"level":"info","ts":"2022-10-27T14:48:36.242-0400","caller":"events/instrumented_project_command_runner.go:53","msg":"apply success. output available at: https://github.com/chronosphereio/atlantis_test/pull/5","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:36.242-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.cmd.comment.apply.execution_time","value":1.009759959,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:36.242-0400","caller":"vcs/github_client.go:175","msg":"POST /repos/chronosphereio/atlantis_test/issues/5/comments","json":{}} {"level":"debug","ts":"2022-10-27T14:48:37.048-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.create_comment.execution_time","value":0.805921834,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:37.048-0400","caller":"events/db_updater.go:25","msg":"updating DB with pull results","json":{"repo":"chronosphereio/atlantis_test","pull":"5"}} {"level":"debug","ts":"2022-10-27T14:48:37.126-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.create_comment.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:37.127-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.update_status.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:37.127-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.cmd.comment.apply.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:37.365-0400","caller":"server/middleware.go:45","msg":"POST /events – from [::1]:65211","json":{}} {"level":"debug","ts":"2022-10-27T14:48:37.366-0400","caller":"events/events_controller.go:98","msg":"handling GitHub post","json":{}} {"level":"debug","ts":"2022-10-27T14:48:37.391-0400","caller":"events/events_controller.go:163","msg":"request valid","json":{"gh-request-id":"X-Github-Delivery=f6ffb5a0-5627-11ed-8a4b-823051ce86cb"}} {"level":"debug","ts":"2022-10-27T14:48:37.391-0400","caller":"server/middleware.go:72","msg":"POST /events – respond HTTP 200","json":{}} {"level":"debug","ts":"2022-10-27T14:48:37.425-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.github.update_status.execution_time","value":0.333658875,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:37.425-0400","caller":"metrics/debug.go:52","msg":"timer","json":{"name":"atlantis.cmd.comment.apply.execution_time","value":3.967310833,"tags":{},"type":"timer"}} {"level":"debug","ts":"2022-10-27T14:48:38.125-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github.update_status.execution_success","value":1,"tags":{},"type":"counter"}} {"level":"debug","ts":"2022-10-27T14:48:38.126-0400","caller":"metrics/debug.go:42","msg":"counter","json":{"name":"atlantis.github_event.comment_created.success_200","value":1,"tags":{},"type":"counter"}} ```

Environment details

Atlantis version:

❯ atlantis version
atlantis 0.20.1

Environment var:

❯ env | grep MERGE
ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true

command:

❯ atlantis server \
--atlantis-url="$URL" \
--gh-user="$USERNAME" \
--gh-token="$TOKEN" \
--gh-webhook-secret="$SECRET" \
--repo-allowlist="$REPO_ALLOWLIST" \
--repo-config=./repos.yaml \
--skip-clone-no-changes \
--log-level=debug

repos.yaml

repos:
- id: /.*/
  allowed_overrides: [apply_requirements]

atlantis.yaml

version: 3
projects:
  - dir: terraform
    apply_requirements: [mergeable]

Github Protected Branch set on Require conversation resolution before merging

Additional Context

Github API spec link
go-github GetCombinedStatus link
atlantis GetCombinedStatusMinusApply link

The text was updated successfully, but these errors were encountered:

nitrocode · 2022-10-27T20:33:54Z

@jffrose thanks for adding this issue. There must have been something missed in this PR #2436

cc: @rayterrill could you take a look?

Somehow atlantis is skipping PullIsMergeable if ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true ?

Perhaps it's in this block that the PR needs to be checked if it's mergeable ?

atlantis/server/events/vcs/github_client.go

Lines 426 to 448 in 5d7c9cf

    
           if g.config.AllowMergeableBypassApply { 
        
           	g.logger.Debug("AllowMergeableBypassApply feature flag is enabled - attempting to bypass apply from mergeable requirements") 
        
           	if state == "blocked" { 
        
           		//check status excluding atlantis apply 
        
           		status, err := g.GetCombinedStatusMinusApply(repo, githubPR, vcsstatusname) 
        
           		if err != nil { 
        
           			return false, errors.Wrap(err, "getting pull request status") 
        
           		} 
        
           		fmt.Printf("Status was %v\n", status) 
        
           		//check to see if pr is approved using reviewDecision 
        
           		approved, err := g.GetPullReviewDecision(repo, pull) 
        
           		if err != nil { 
        
           			return false, errors.Wrap(err, "getting pull request reviewDecision") 
        
           		} 
        
           		//if all other status checks EXCEPT atlantis/apply are successful, and the PR is approved based on reviewDecision, let it proceed 
        
           		if status && approved { 
        
           			return true, nil 
        
           		} 
        
           	} 
        
           }

Perhaps it's doing what was implemented ? Bypassing the mergeable requirements ? 🤔

rayterrill · 2022-10-28T17:24:49Z

I've moved on to a new gig where I don't have an easy place to test this at the moment. I would need to build up a test environment for atlantis.

Unfortunately, this is sort of the issue with this functionality - we're trying to back into a configuration that github doesn't actually support, with ample opportunity to miss something along the way because there are so many moving parts to how this works (and github has a number of apis that need to be consulted to see the actual "status" of a PR).

@jffrose what does the github GUI look like in the state where it would allow you to apply?

jffrose · 2022-10-28T18:08:04Z

Merging is blocked but then atlantis apply still passes

rayterrill · 2022-10-28T18:49:35Z

My guess is the issue is in here:

atlantis/server/events/vcs/github_client.go

Lines 288 to 345 in 9ab3fd2

    
           // GetCombinedStatusMinusApply checks Statuses for PR, excluding atlantis apply. Returns true if all other statuses are not in failure. 
        
           func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *github.PullRequest, vcstatusname string) (bool, error) { 
        
           	//check combined status api 
        
           	status, _, err := g.client.Repositories.GetCombinedStatus(g.ctx, repo.Owner, repo.Name, *pull.Head.Ref, nil) 
        
           	if err != nil { 
        
           		return false, errors.Wrap(err, "getting combined status") 
        
           	} 
        
           	//iterate over statuses - return false if we find one that isnt "apply" and doesnt have state = "success" 
        
           	for _, r := range status.Statuses { 
        
           		if strings.HasPrefix(*r.Context, fmt.Sprintf("%s/%s", vcstatusname, command.Apply.String())) { 
        
           			continue 
        
           		} 
        
           		if *r.State != "success" { 
        
           			return false, nil 
        
           		} 
        
           	} 
        
           	//get required status checks 
        
           	required, _, err := g.client.Repositories.GetBranchProtection(context.Background(), repo.Owner, repo.Name, *pull.Base.Ref) 
        
           	if err != nil { 
        
           		return false, errors.Wrap(err, "getting required status checks") 
        
           	} 
        
           	//check check suite/check run api 
        
           	checksuites, _, err := g.client.Checks.ListCheckSuitesForRef(context.Background(), repo.Owner, repo.Name, *pull.Head.Ref, nil) 
        
           	if err != nil { 
        
           		return false, errors.Wrap(err, "getting check suites for ref") 
        
           	} 
        
           	//iterate over check completed check suites - return false if we find one that doesnt have conclusion = "success" 
        
           	for _, c := range checksuites.CheckSuites { 
        
           		if *c.Status == "completed" { 
        
           			//iterate over the runs inside the suite 
        
           			suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), repo.Owner, repo.Name, *c.ID, nil) 
        
           			if err != nil { 
        
           				return false, errors.Wrap(err, "getting check runs for check suite") 
        
           			} 
        
           			for _, r := range suite.CheckRuns { 
        
           				//check to see if the check is required 
        
           				if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) { 
        
           					if *c.Conclusion == "success" { 
        
           						continue 
        
           					} else { 
        
           						return false, nil 
        
           					} 
        
           				} else { 
        
           					//ignore checks that arent required 
        
           					continue 
        
           				} 
        
           			} 
        
           		} 
        
           	} 
        
           	return true, nil 
        
           }

. We're trying to back into whether or not the PR should be allowed to proceed, trying to look at everything EXCEPT the atlantis/apply check. Specifically I bet something needs to be added in here:

atlantis/server/events/vcs/github_client.go

Lines 319 to 342 in 9ab3fd2

    
           for _, c := range checksuites.CheckSuites { 
        
           	if *c.Status == "completed" { 
        
           		//iterate over the runs inside the suite 
        
           		suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), repo.Owner, repo.Name, *c.ID, nil) 
        
           		if err != nil { 
        
           			return false, errors.Wrap(err, "getting check runs for check suite") 
        
           		} 
        
           		for _, r := range suite.CheckRuns { 
        
           			//check to see if the check is required 
        
           			if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) { 
        
           				if *c.Conclusion == "success" { 
        
           					continue 
        
           				} else { 
        
           					return false, nil 
        
           				} 
        
           			} else { 
        
           				//ignore checks that arent required 
        
           				continue 
        
           			} 
        
           		} 
        
           	} 
        
           }

to check for this specific scenario (and other similar ones) - must be missing now.

I'll try to set up an Atlantis test case this wknd.

github-actions · 2023-03-12T02:03:44Z

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.'

tpolekhin · 2023-03-17T22:58:32Z

@nitrocode I am willing to look at this issue, because I think it breaks a desired workflow. Not sure how to attack it though, except for adding additional checks in the PullIsMergeable function for all the different "blockers" of the PR merge. If you have any ideas please share.

nitrocode · 2023-03-18T01:58:27Z

@tpolekhin that sounds exactly like what we'd need to do. Please propose a pr. We'd love to see this get into the repo 😄

stasostrovskyi · 2023-04-23T14:01:34Z

Seems like Repository Rules feature from GitHub can solve this issue, see my comment

jffrose added the bug Something isn't working label Oct 27, 2022

jamengual added the waiting-on-response Waiting for a response from the user label Oct 28, 2022

nitrocode changed the title ~~-gh-allow-mergeable-bypass-apply skips status checks~~ --gh-allow-mergeable-bypass-apply skips status checks Nov 8, 2022

github-actions bot added the Stale label Mar 12, 2023

nitrocode removed waiting-on-response Waiting for a response from the user Stale labels Mar 18, 2023

dosubot bot added the Stale label Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--gh-allow-mergeable-bypass-apply skips status checks #2624

--gh-allow-mergeable-bypass-apply skips status checks #2624

jffrose commented Oct 27, 2022 •

edited

Loading

nitrocode commented Oct 27, 2022 •

edited

Loading

rayterrill commented Oct 28, 2022

jffrose commented Oct 28, 2022

rayterrill commented Oct 28, 2022 •

edited

Loading

github-actions bot commented Mar 12, 2023

tpolekhin commented Mar 17, 2023

nitrocode commented Mar 18, 2023

stasostrovskyi commented Apr 23, 2023

--gh-allow-mergeable-bypass-apply skips status checks #2624

--gh-allow-mergeable-bypass-apply skips status checks #2624

Comments

jffrose commented Oct 27, 2022 • edited Loading

Community Note

Overview of the Issue

Reproduction Steps

Logs

Environment details

Additional Context

nitrocode commented Oct 27, 2022 • edited Loading

rayterrill commented Oct 28, 2022

jffrose commented Oct 28, 2022

rayterrill commented Oct 28, 2022 • edited Loading

github-actions bot commented Mar 12, 2023

tpolekhin commented Mar 17, 2023

nitrocode commented Mar 18, 2023

stasostrovskyi commented Apr 23, 2023

jffrose commented Oct 27, 2022 •

edited

Loading

nitrocode commented Oct 27, 2022 •

edited

Loading

rayterrill commented Oct 28, 2022 •

edited

Loading