Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
asantos-fuze opened this issue Mar 8, 2019 · 31 comments
Closed

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

asantos-fuze opened this issue Mar 8, 2019 · 31 comments
Labels
feature New functionality/enhancement

Comments

@asantos-fuze
Copy link

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
@lkysow
Copy link
Member

lkysow commented Mar 8, 2019

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.

@lkysow
Copy link
Member

lkysow commented Mar 8, 2019

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

@chrisob
Copy link
Contributor

chrisob commented Mar 8, 2019

👍 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?

@lkysow
Copy link
Member

lkysow commented Mar 8, 2019

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 #523 (comment). 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.

@chrisob
Copy link
Contributor

chrisob commented Mar 9, 2019

@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:

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

@asantos-fuze
Copy link
Author

asantos-fuze commented Mar 11, 2019

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

@pratikmallya
Copy link
Contributor

pratikmallya commented Mar 19, 2019

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.

@majormoses
Copy link
Contributor

@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.

@lkysow lkysow added the feature New functionality/enhancement label Apr 8, 2019
@wyardley
Copy link
Contributor

wyardley commented Apr 9, 2019

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

image

yet:

image

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

@lkysow
Copy link
Member

lkysow commented Apr 10, 2019

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:
image

[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?

@wyardley
Copy link
Contributor

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.

@danielmotaleite
Copy link

@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?

@lkysow
Copy link
Member

lkysow commented Aug 20, 2019

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

@danielmotaleite
Copy link

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

@lkysow
Copy link
Member

lkysow commented Sep 6, 2019

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

Thanks for trying!

@nishkrishnan
Copy link
Contributor

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:

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

@majormoses
Copy link
Contributor

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.

@rawlbot
Copy link

rawlbot commented Mar 31, 2021

@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.

@nishkrishnan
Copy link
Contributor

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.

@rawlbot
Copy link

rawlbot commented Mar 31, 2021

@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.

@Sergey-Kirgizov
Copy link

Sergey-Kirgizov commented Jun 19, 2021

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?

@akobir-mp
Copy link

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

@nishkrishnan
Copy link
Contributor

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

@seany89
Copy link

seany89 commented Jan 19, 2022

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

@dwilliams782
Copy link

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

@rayterrill
Copy link
Contributor

rayterrill commented Aug 2, 2022

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.

@rayterrill
Copy link
Contributor

New PR building on the work of others: #2436

@nitrocode
Copy link
Member

Is this still an issue or can this be closed due to merging of #2436?

@nitrocode
Copy link
Member

You can now add atlantis/apply as a required check

Set https://www.runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply flag

@ChrisJBurns
Copy link

https://www.runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply

This doesn't work for Gitlab as it seems to be a Github specific flag.

@nitrocode
Copy link
Member

Please create a separate issue for other VCSs. Feel free to propose a PR too. It would be nice to fix this for all VCS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement
Projects
None yet
Development

No branches or pull requests