fix: the logic when checking the check suites

[Yasmine92]

what

this is an aim to fix #3811

why

as explained in the issue, Atlantis is able to apply when the required checks are still in progress. it seems to me that the logic is broken in the GetCombinedStatusMinusApply function , as it will return true when c.Status =! "completed"

tests

I have tested my changes by building Atlantis with the update I included, and built a custom image that I used in the same context and workflow, and Atlantis seemed to behave as expected.

references

• closes
  Atlantis able to apply when apply requirements are set to mergable, and the required checks status are in progress #3811

[stasostrovskyi]

This is not always correct because a check can be in progress but it may as well be not set as required, in which case PR will still be mergeable.

[GenPage]

This is checked prior to the function call here:

atlantis/server/events/vcs/github_client.go

Lines 497 to 534 in 8f97ded

	state := githubPR.GetMergeableState()
	// We map our mergeable check to when the GitHub merge button is clickable.
	// This corresponds to the following states:
	// clean: No conflicts, all requirements satisfied.
	// Merging is allowed (green box).
	// unstable: Failing/pending commit status that is not part of the required
	// status checks. Merging is allowed (yellow box).
	// has_hooks: GitHub Enterprise only, if a repo has custom pre-receive
	// hooks. Merging is allowed (green box).
	// See: https://github.com/octokit/octokit.net/issues/1763
	if state != "clean" && state != "unstable" && state != "has_hooks" {
	//mergeable bypass apply code hidden by feature flag
	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")
	}
	//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
	}
	}
	}
	return false, nil
	}
	return true, nil
	}

The state you are referring to is considered "unstable"

[bhavith]

I think @stasostrovskyi is right here. Assume atlantis/apply is pending and one another check which is not a required one is also pending. So the status will not be unstable and will go on to the function GetCombinedStatusMinusApply since AllowMergeableBypassApply is enabled.

There the changes that are done in this PR will come into picture. Here it will fail the check if *c.Status == "completed" and proceed to to return return false, nil, even before checking if the status check is failing due to a required check or not.

I think we might have to go further if *c.Status != "completed" and then check if the checkrun is a required one or not.

Does that make sense?

[stasostrovskyi]

For me it finally makes sense with the issue you've discovered and fixing in #3916

[GenPage]

Can we update the tests? I think they start around here:

atlantis/server/events/vcs/github_client_test.go

Line 589 in 8f97ded

func TestGithubClient_PullIsMergeableWithAllowMergeableBypassApply(t *testing.T) {

[GenPage]

Might help to review #2436 as a reference for the feature.

[jamengual]

@Yasmine92 do you have time to work on this?

[Yasmine92]

@Yasmine92 do you have time to work on this?

@jamengual Unfortunately not, I apologize for not coming back to this one. I need time to understand well the logic of the github checks suite and how it's explored here.
if anyone knows how to fix it, feel free to commit changes to my branch.

[github-actions]

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.

[GenPage]

Closing in favor of #3916