atlantis Atlantis v0.5.0 with apply_requirements: [mergeable] wont work

Hi,

I'm testing the latest release of Atlantis, v0.5.0 and have enabled the new check on the repo for apply/atlantis and apply/atlantis. After enabling those I've found out that after a successfully plan, when commenting on the PR with atlantis apply I am not able to merge branch if I have the setting apply_requirements: [mergeable] configured on atlantis.yaml If I disable everything works.

Issue New Atlantis checks incompatible with apply requirement == mergeable

Expected Behaviour Expected new checks not to impact mergeable requirement

Atlantis Plan - OK screenshot-github com-2019 03 08-10-54-11

Atlantis Apply - Fails screenshot-github com-2019 03 08-10-55-15

atlantys.yaml config

version: 2
automerge: true
projects:
  - name: poc-dev
    dir: poc-aws-roles-poc/poc-dev/terraform
    apply_requirements: [mergeable]
    autoplan:
      when_modified: ["../dev-policies/*.terraform", "*.tf", "*.tfvars"]
    workflow: landscape

Mar 08 '19 11:03 asantos-fuze

Yeah the mergeable check won't work with the status checks unfortunately. There's no good way to solve it either because GitHub doesn't tell us why a pull request isn't mergeable and we can't do:

check if pull is mergeable
if not, check if atlantis statuses aren't passing
if atlantis statuses aren't passing, assume that's the reason it's not mergeable and continue

I think we'll have to add some new apply requirements like checks_passing and codeowners_approved which are more fine-grained.

Mar 08 '19 14:03 lkysow

Right now I should at least add a disclaimer to our docs.

Mar 08 '19 17:03 lkysow

:+1: on this... There is now no way to enforce that atlantis apply is run before a PR is merged if:

apply_requirements: [mergeable] is set
apply/atlantis status check is required under the repo's branch protection

I'm not so familiar with the code, but would it be reasonable/possible to rework the PullIsMergeable method to exclude the apply/atlantis status check from the mergeable check?

Mar 08 '19 18:03 chrisob

I'm not so familiar with the code, but would it be reasonable/possible to rework the PullIsMergeable method to exclude the apply/atlantis status check from the mergeable check?

No, unfortunately not, see https://github.com/runatlantis/atlantis/issues/523#issuecomment-470950373. It's a single API call to GitHub to find out if the pull is mergeable.

@chrisob and @asantos-fuze what do you use the mergeable check to check for? We'll have to pull those into separate checks.

Mar 08 '19 19:03 lkysow

@lkysow We check that:

Required status checks pass (excluding apply/atlantis, although we still want this to be required from GH's perspective before actually merging a PR). IMHO, this is the most important.
Number of required approvals have been given (this one isn't so important for us, because we have a custom status check to require that certain people/teams have approved the PR).
There are no conflicts with the base branch.
CODEOWNERS have given their approval (probably covered by required approvals above).

We don't care if Atlantis is actually able to merge the PR to the base branch or not. In other words, Atlantis doesn't need to see the big green merge button to consider the PR mergeable.

From my perspective, all the usual mergeability checks would be nice, only excluding:

The apply/atlantis status check (even if GH considers it a required status check under branch protection)
That the Atlantis GH user sees the big green merge button (ie. has write/push permission to the base branch).

Mar 09 '19 14:03 chrisob

The checks for mergeable would stop the atlantis apply to be run before the mergeable condition is met.

Without the mergeable check configurations start to drift.

The example is this:

Create PR with conflict
atlantis plan -> check turns ok
atlantis apply -> check turns ok
auto merge fails
changes contained on the PR where apply on the target infra
master is different from the provisioned infra
PR is still not merged

Mar 11 '19 10:03 asantos-fuze

There is a somewhat easy way around this in Github at least.

enable the Github branch protection option: 'Restrict who can push to matching branches' to only the atlantis github svc account and
enable the merge on apply option for atlanits
leave the apply/atlantis check as NOT REQUIRED

This will:

allow atlantis apply to run because apply/Atlantis is not a required status check
prevent anyone from merging branch except atlantis svc account
atlantis svc account can merge any changes once all plans are applied

This setup allows you to not use required check for apply/Atlantis while continuing to be able to:

gate merges on successful applies of all plans and
allow applies to run even though apply success is required

i.e. the merge only if apply succeeds condition is met by the auto-merge option, which will merge iff all the plans are applied.

Mar 19 '19 06:03 pratikmallya

@pratikmallya thanks for some thoughts on work arounds in the meantime and that's what I am about to do.

I spoke with @lkysow and we think that the best fix would be during the apply stage to send an ok/passing status to atlantis/apply prior to checking its mergability, in the case that mergability still fails it should rescue and set the status of that one to false to prevent someone from accidentally merging it.

Mar 28 '19 18:03 majormoses

I'm getting this error, and even though atlantis/apply is not 'required', and all required status checks are passing, it's still barfing.

yet:

[requiring approval instead hasn't been working for me so far]

Apr 09 '19 23:04 wyardley

I'm getting this error, and even though atlantis/apply is not 'required', and all required status checks are passing, it's still barfing.

yet:

[requiring approval instead hasn't been working for me so far]

To debug, log in as the Atlantis user and look at the pull request, is the merge button clickable?

Apr 10 '19 14:04 lkysow

To debug, log in as the Atlantis user and look at the pull request, is the merge button clickable?

I see - haven't tested that, but that could be the issue, it may (intentionally) not be in a group that can merge.

Apr 11 '19 04:04 wyardley

@majormoses @lkysow Sorry to nag, but any new on this?

we have to choose between (@pratikmallya method) not require the apply (but admins may merge by accident without applying and not changing the resources) or allow the apply without mergeability (and allow one to apply outdated info)

How about contact github, maybe they could extend the api to allow this checks in a better way?

Aug 20 '19 14:08 danielmotaleite

Hi Daniel, no, no news. I don't think contacting GitHub would be productive but you could try?

Aug 20 '19 14:08 lkysow

i wrote to github and support said it would forward it internally... but of course, no promises nor ETA

Sep 05 '19 17:09 danielmotaleite

i wrote to github and support said it would forward it internally... but of course, no promises nor ETA

Thanks for trying!

Sep 06 '19 09:09 lkysow

So the workaround for this is to send a passing status for the apply check?

Have we considered running the following logic as a fallback to checking PR mergeability:

Call for all the status checks associated with the ref
Check that everything is passing except the apply status check(s)

Oct 01 '20 23:10 nishkrishnan

I have not made any progress on this but I have been running removing the apply from my branch protection without issue since we discovered that work around at my org. The biggest problem I have seen is that people are more likely to accidentally merge the PR without running the apply.

I did think of another issue with the approach, if that fails how does one recover? If we expose a command for it then I would want to have RBAC in place as a requirement. I said fairly early on in the project RBAC should be something we solve later as it needed to focus on other areas. I think we are approaching the point where we really need to start considering how/when to tackle that.

Oct 17 '20 20:10 majormoses

@majormoses which work around are you implementing?

I've started to hit this issue and I'm nervous about removing the apply check as we have already had to deal with changes being merged without them being applied. However, as we grow our validation checks we are learning that not having mergeable set means someone could apply changes that have failed other (non Atlantis) checks since Atlantis is only looking at itself when mergeable is not set.

Mar 31 '21 16:03 rawlbot

This was our work around since we have an org wide status check we wanted atlantis to ignore in addition to atlantis/apply: https://github.com/lyft/atlantis/blob/release-v0.17.0-beta-lyft.1/server/events/vcs/github_client.go#L296

If this gets enough likes I can merge just the part about atlantis/apply to the upstream.

Mar 31 '21 16:03 nishkrishnan

@nishkrishnan awesome! ty for sharing.

I would totally support you merging the parts of atlantis/apply up as that's likely the most useful for everyone. Perhaps even in a way that's configurable via the existing configs.

Mar 31 '21 18:03 rawlbot

Is there any changes on this issue? I am surprisingly starting to hit this one since Thursday. I wonder how I was able to not hit this issue before and what has been changed on Thursday, June 17, 2021?

Jun 19 '21 08:06 Sergey-Kirgizov

This was our work around since we have an org wide status check we wanted atlantis to ignore in addition to atlantis/apply: https://github.com/lyft/atlantis/blob/release-v0.17.0-beta-lyft.1/server/events/vcs/github_client.go#L296

If this gets enough likes I can merge just the part about atlantis/apply to the upstream.

hey @nishkrishnan any chance you can merge this? seems like a pretty common usecase that a lot of folks are running into

Sep 28 '21 16:09 akobir-mp

yep sorry been swamped with work lately, will try to get this in the next week or so.

Oct 14 '21 18:10 nishkrishnan

This seems to still be an issue using the latest version v0.18.1

Jan 19 '22 13:01 seany89

Agreed - I think the new log streaming status is not being excluded @nishkrishnan.

Feb 23 '22 12:02 dwilliams782

Looks like this is still an issue in v0.19.3. @nishkrishnan looks like you added support for filtering out apply from the mergeability, but then it was reverted with #1968? Looks like there was a regression associated with that that allowed unapproved plans to be applied.