fix: Include required checks while evaluating the mergeable state if apply is bypassed

[bhavith]

what

Currently when the apply condition is set to mergeable along with --gh-allow-mergeable-bypass-apply flag, Atlantis considers non required status checks as well to decide whether the PR is mergeable or not (to allow apply or not). This is a deviation from standard github mergeable status.

This PR changes that and includes only required status checks while evaluating mergeable status.

why

Atlantis uses mergeable status from github API to check if a PR is mergeable if --gh-allow-mergeable-bypass-apply is not set. That is if the status is unstable, the PR is considered mergeable.

unstable: Failing/pending commit status that is not part of the required
	//           status checks. Merging is allowed (yellow box).

If --gh-allow-mergeable-bypass-apply enabled, github uses some additional checks to see if the PR is mergeable without considering atlantis apply. But in these additional checks, Atlantis looks at all statuses irrespective of if they are required or not. So, if there is a status check that is failing Atlantis will consider it non mergeable even if its not a required status check and thus will block Apply.

In this change, while checking if the state is success, it will also check if the particular status is one of the required checks. Hence Atlantis will report non mergeable only if the failing status check is a required one.

tests

• [x] I have adjusted the test data to include scenarios where there could be failing non required checks
• [x] I have tested this by running atlantis locally against a terraform repository with a few valid scenarios

[jamengual]

@lukemassa do you happen to have any input on this?

[lukemassa]

I am much less familiar with github's terminology/expectations around checks, but this logic seems reasonable to me. Right now it seems like we're only considered required vs not for check suites, whereas this change makes it so we look at required vs not for combined check status as well.

[bhavith]

Hi @jamengual, Anything needed from my end to take this forward? I am kinda new here as a contributor :) so not sure how exactly it works.

[jamengual]

Hi @bhavith.

We need to review the code when we find some time.

We will get back to you as soon as we can.

Thanks for the contribution.

[bhavith]

perfect! 👍

[brandon-fryslie]

We have run into this a quite a few times and weren't sure what was going on. Here's the scenario:

• GitHub + Jenkins + Jenkins Branch Source Plugin (automatically creates a separate job per branch and per PR)
• A user pushes a new branch. This kicks off a job. The job sets a commit status (e.g., branch-build). This is NOT a required check
• Before the branch build finishes, the user opens a PR from the branch. This check IS required. This job kills the branch build, leaving it in a failure state. We don't care about the status of branch builds, only PR builds
• Atlantis won't auto-merge the PR as expected because apply is a required check and we have --gh-allow-mergeable-bypass-apply enabled.

This seems to happen intermittently rather than consistently, but it's possible it's only working when either a) the branch build never starts because a PR is opened immediately, or b) the branch build has finished successfully before the PR build begins.

[stasostrovskyi]

It will probably make sense to combine this PR with this change https://github.com/runatlantis/atlantis/pull/3812 to make the whole feature even more reliable.

[GenPage]

Looks like the PR author of #3812 doesn't have time to work on his PR, please feel free to merge both in this PR.

[bhavith]

Will have a look at #3812 and get back.

[bhavith]

Looks like the PR author of #3812 doesn't have time to work on his PR, please feel free to merge both in this PR.

I will make some changes in this PR based on https://github.com/runatlantis/atlantis/pull/3812#discussion_r1428584337. That will be in 2024. Happy holidays. 🎄

[github-actions[bot]]

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]

@bhavith any update?

[bhavith]

@bhavith any update?

Came back from vacation sick and weather in Norway wasn't very helpful either. I will try to work on it sometime this week/weekend.

[stasostrovskyi]

@bhavith Our team was trying to fix support for github rulesets and the solution turned out to be overlapping with your PR. Please, take a look at #4193 and let us know what you think

[bhavith]

@bhavith Our team was trying to fix support for github rulesets and the solution turned out to be overlapping with your PR. Please, take a look at #4193 and let us know what you think

Nice. That PR looks like a more comprehensive fix including mine and #3812 . I will follow that PR.

[github-actions[bot]]

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.