atlantis icon indicating copy to clipboard operation
atlantis copied to clipboard

Feature request: allow apply on merge

Open jacekn opened this issue 2 years ago • 34 comments

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

Currently when a PR is merged without atlantis apply we end up in a situation where reality doesn't match desired state described with terraform. There is also no way to apply changes post-merge without raising a dummy PR.

At the same time, in GitHub, it's sometimes not possible to prevent people form merging PRs. For example anyone with repo admin access will get implicit push access and thus will be able to merge approved PRs without having to apply first.

In practice this means that there is currently no way to prevent human errors (people merging approved PRs without running atlantis apply first).

I'd like to propose optional functionality to allow atlantis to be configured to apply changes on PR merges. This would allow organizations who cannot remove push or admin access from repos to ensure changes are applied on merges without relying on humans to remember to atlantis apply

I realize that there is a possibility that apply fails and we'll end up being out of sync but this is a trade off that some organizations might be willing to accept.

Reproduction Steps

Open a PR and then merge it without running atlantis apply first. This will result in code being out of sync with reality and there is no way to retroactively apply changes. The only workaround I found is to open 2nd dummy PR and ensure it's applied before merging.

Logs

Environment details

I'm on atlantis v 0.18.2 but I believe older/newer versions will also be affected by this.

Additional Context

This feature was requested in 2017 but wasn't implemented back then: https://github.com/runatlantis/atlantis/issues/36

There is also discussion about the mergable limitations here: https://github.com/runatlantis/atlantis/issues/1316

Apply on merge would allow us to create workflow similar to that hashicorp describe in a docs. This makes me think that the functionality is non-controversial as hashicorp themselves describe it.

jacekn avatar Mar 30 '22 13:03 jacekn

will setting atlantis/apply as a required check in branch protection rules help in that case? note: might be only available on latest version image

edbighead avatar Apr 06 '22 09:04 edbighead

will setting atlantis/apply as a required check in branch protection rules help in that case? note: might be only available on latest version image

No it won't work. For atlantis to respect GitHub branch protection rules we need to use mergable apply requirement. If we set up branch protection rule to require successful atlantis/apply then the PR will not become mergable until we apply and we can't apply until it's mergable...

jacekn avatar Apr 06 '22 10:04 jacekn

Hey all, To add this feature can we do something like this (Please correct me if I am wrong)? :

  • Add a support for a flag apply-on-merge in the atlantis server with which users can specify if they want to enable apply post merge. Add pull_request_event type as merged to PullRequestEventType in server/events/models/models.go. Refer go-github API code
  • Add condition to allow atlantis to apply when there's flag apply-on-merge is set to true and PR is merged.
  • If flag apply-on-merge is set to true, bypass the rule for checking the PR state ctx.Pull.State != models.OpenPullState in the server/events/command_runner.go.
  • Make sure that post run hooks (server/events/post_workflow_hooks_command_runner.go) uses flag apply-on-merge to decide not to merge the PR (since its already merged) and continue with removing the atlantis locks.
  • If state of the PR is open and if atlantis apply comment event occurs, check the flag apply-on-merge and then reject the apply since flag apply-on-merge flag is set.
  • If user comments atlantis apply on the closed PR, return as usual.

There can be an alternate atlantis requirement for apply-on-merge flag. So that users can specify that via config file. Does this make sense? What else can we improve in this to achieve apply post merge?

Thanks.

satyamz avatar Apr 18 '22 06:04 satyamz

Would one of the maintainers be able to access above plan form @satyamz ? We're considering contributing to the project by adding this functionality but would like to know if the plan looks sound and whether a PR would be accepted.

jacekn avatar Apr 21 '22 10:04 jacekn

I'm interested in this due to the GitLab API bug mentioned in #1174 that is preventing Atlantis' mergeable apply requirement from functioning. Having apply-on-merge would let us make sure all the MR approvals are adhered to before the plan can be applied.

jghal avatar Apr 26 '22 15:04 jghal

I'd like to take this issue one step further:

automatically plan on any change to the PR - failing on other parallel PRs is okay - but maybe make that optional for obvious reasons:

do NOT respond to atlantis apply manual commands on unmerged files

and finally, apply on merge

this relies on the plan being okay and the git provider approvals/results on the PR being acceptable and merge possible - so no smarts needed there, just dump plan output on every push and do apply and dump output on merge.

This tool is almost perfect on the face of it but the above work flow is the only one that makes sense to me for any serious use.

fredcooke avatar May 26 '22 07:05 fredcooke

I'd like to take this issue one step further:

automatically plan on any change to the PR - failing on other parallel PRs is okay - but maybe make that optional for obvious reasons:

I believe plans are regenerated on PR changes already, there is no need to atlantis plan when new commits are added.

do NOT respond to atlantis apply manual commands on unmerged files

Yes absolutely, it's a good point that in this scenario atlantis apply should not do anything.

and finally, apply on merge

this relies on the plan being okay and the git provider approvals/results on the PR being acceptable and merge possible - so no smarts needed there, just dump plan output on every push and do apply and dump output on merge.

Yes exactly, this is exactly the workflow I'd like to use as well!

This tool is almost perfect on the face of it but the above work flow is the only one that makes sense to me for any serious use.

It's good to see that there are more people who have the same requirements!

jacekn avatar May 26 '22 11:05 jacekn

We would love something along these lines as feature, as this is something that happens on a regular basis with engineers new to our team, or for engineers from outside our organisation.

The ability to require the atlantis/apply check to pass for users to merge, and for atlantis to consider a PR mergeable if all required checks except for that check pass, would likely be enough for us.

iandelahorne avatar Jun 06 '22 23:06 iandelahorne

we would love to see this feature implemented for our team as well, many people forgot to comment atlantis apply and straight merged the PR. And we can't set the atlantis/apply status check due to the conflict with mergeable

zepeng811 avatar Aug 03 '22 20:08 zepeng811

Speaking as an interested observer, I'm curious if there's something about Atlantis' design that requires it to only run during the PR phase. I ask because I recently came across a notice that said a failure occurring post-merge "would need" to be fixed by a subsequent PR. Is this really mandatory?

If I have a GitHub Action that fails post-merge, I have the option to just re-run it. Granted, if the fault is something within the repository that should never have been merged I'll most likely fix it in a new PR. But if the issue lies outside the repo, if it's something like an expired token or an intermittent network outage, I just need a button.

Is there something fundamental to Atlantis that prevents this sort of capability?

siggimoo avatar Aug 03 '22 22:08 siggimoo

is this still an issue with v0.19.8?

jamengual avatar Aug 26 '22 02:08 jamengual

is this still an issue with v0.19.8?

As far as I can tell yes this is still a problem. The tricky part about "apply on merge" functionality is that it's mostly GitHub limitation that we are dealing with. It is currently not possible to confirm whether GitHub merge requirements were met without actually allowing people to merge. And once people merge we have a drift between code and actual state.

jacekn avatar Sep 05 '22 11:09 jacekn

Add atlantis/apply as a required check

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

nitrocode avatar Jan 17 '23 16:01 nitrocode

@nitrocode regardless of the potential workaround I still think this feature would be very useful. Based on thumbs up there are at least 44 others who agree.

We actually added this capability to forked atlantis, we should be in a position to submit a PR soon so that everyone in the community can benefit from it.

Could this issue be reopened?

jacekn avatar Jan 17 '23 18:01 jacekn

this can be reopened but keep in mind that people tried to do this before in different ways and a lot of logic was added and broke other functionality since this part of the code is very tricky due to workarounds and limitations on VCSs.

PRs are welcome but please give it a try to the gh-allow-mergeable-bypass-apply that @nitrocode mentioned.

jamengual avatar Jan 17 '23 18:01 jamengual

I don't understand what is wrong with the current solution or workaround of using https://www.runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply. Please explain why this method is not what you prefer @jacekn

nitrocode avatar Jan 18 '23 14:01 nitrocode

@nitrocode Adding this flag still doesn't work, unless there's some other setting I'm missing. I've setup the atlantis apply check and adding the flag, but after getting an approval I get this message. Once I remove the atlantis/apply check, I'm able to apply normally.

Setting the flag

    - name: ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY
      value: "true"

This is what we have for our apply_requirements

      apply_requirements: [approved, mergeable]

Apply Failed: Pull request must be approved by at least one person other than the author before running apply.

Manan-Kothari avatar Feb 17 '23 17:02 Manan-Kothari

@Manan-Kothari

Apply Failed: Pull request must be approved by at least one person other than the author before running apply.

You have mergeable set and you have PR requirements of 1 approval or more... so you need an approval on the PR before you can merge the PR via atlantis apply.

nitrocode avatar Feb 17 '23 23:02 nitrocode

I did have that which is why I'm asking

image

Manan-Kothari avatar Feb 20 '23 15:02 Manan-Kothari

I don't understand what is wrong with the current solution or workaround of using https://www.runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply. Please explain why this method is not what you prefer @jacekn

Here are the reasons I can think of:

  1. If atlantis just ignores "atlantis/apply" status check then we would require users to know internals of atlantis and know that changes can be applied without this particular status check. Basically there would be no green "merge" button to indicate all requirements are met.
  2. It's not unusual for projects to act on new commits in the main/master branch. For example those can trigger builds or deploys to dev. With this workflow being used in an organization it wold be nice for atlantis to behave in the same way. It's a matter of choice of course. Driving things from GH comments is a valid method to interact with systems too.
  3. The bypass method does not allow admins to restrict who can land changes. With apply on merge admins can control, via GitHub repo perms, who can merge changes thus apply them. People who can apply can be subset of those who can open PRs
  4. The "apply on merge" workflow is what hashicorp describe, for example here. It would be nice to be able to replicate this workflow in atlantis without the need for terraform cloud. Given that the workflow is described by hashicorp I think this workflow should not be considered controversial.

jacekn avatar Feb 20 '23 16:02 jacekn

@nitrocode quick question about the --gh-allow-mergeable-bypass-apply flag as it's not clear from the docs. With the flag set will atlantis ensure that all non-status-check branch protection rules are passing? For example branches might require conversation resolution or signed commits. Will those be taken into account and will atlantis only allow changes to be applied if all rules are met?

jacekn avatar Feb 21 '23 09:02 jacekn

@jacekn number 4 exactly - terraform cloud works quite nicely in this regard, though it'd be nice to have some alternative to it that isn't proprietary and pay per view.

This stuff is trivial to achieve in something like Jenkins. You need some sort of call back event or webhook on new updates to the default branch ref spec in the remote - eg direct push or PR merge of any kind.

The comment in the docs about lack of status checks in the branch protection rules seems bizarre at first - you can configure them to require any/all status checks and then when some kind of CI/CD registers one against it, suddenly that becomes required, for example.

PR created = plan Any update to default branch refspec = apply

How hard can it be? I don't understand how that option helps at all, @nitrocode / @jamengual ? Seems to be orthogonal to the request here.

fredcooke avatar Feb 21 '23 09:02 fredcooke

we are not opposed to add a new functionality that can solve this problem.

there is many ways mentioned that try in one way or another to solve this issue but it looks like at the end of the day something like - - apply-on-merge flag is necessary for those admins that do not like to apply their changes before merging.

jamengual avatar Feb 21 '23 17:02 jamengual

disclaimer: i did not yet install or use atlantis, rather waiting for this feature to be implemented before trying out.

question: could apply-on-merge be somehow achieved with existing atlantis version, if we would trigger some github action on "push to master" which would then do an api call to atlantis? in this case it could be possible to say that merge condition is "atlantis plan passed without errors, and codeowner gave approval to merge" (that would be taken care of completely at github side) and then after merge, push to master would happen triggering "something" via atlantis http api. I can imagine that main problem could be to somehow infer which exactly prepared plan needs to be run at atlantis side, and then it would just boild down to streaming atlantis api output as github action output for real-time progress report and merge result outcome (pass|errored)

ervin-pactum avatar Feb 23 '23 08:02 ervin-pactum

yes, that is possible.

On Thu, Feb 23, 2023, 12:39 a.m. Ervin Weber @.***> wrote:

disclaimer: i did not yet install or use atlantis, rather waiting for this feature to be implemented before trying out.

question: could apply-on-merge be somehow achieved with existing atlantis version, if we would trigger some github action on "push to master" which would then do an api call to atlantis? in this case it could be possible to say that merge condition is "atlantis plan passed without errors, and codeowner gave approval to merge" (that would be taken care of completely at github side) and then after merge, push to master would happen triggering "something" via atlantis http api. I can imagine that main problem could be to somehow infer which exactly prepared plan needs to be run at atlantis side, and then it would just boild down to streaming atlantis api output as github action output for real-time progress report and merge result outcome (pass|errored)

— Reply to this email directly, view it on GitHub https://github.com/runatlantis/atlantis/issues/2172#issuecomment-1441380006, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQ3ERHFBI574ZE2BFHESU3WY4O5ZANCNFSM5SCAIYHA . You are receiving this because you were mentioned.Message ID: @.***>

jamengual avatar Feb 23 '23 16:02 jamengual

still an issue with 0.27.2

@jacekn I tried to answer your questions here: https://github.com/runatlantis/atlantis/issues/2172#issuecomment-1438098475 (i.e have a required atlantis/apply status check + atlantis having ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY set to true + apply_requirements: [approved, mergeable, undiverged]), and it didn't respect the fact that there was no approving PR review in the branch protection rule :/

cc @nitrocode

dimisjim avatar Mar 22 '24 15:03 dimisjim

@dimisjim I think you need to open a separate bug for your case and attach your branch protection configuration. Note that currently that flag in Atlantis only works with branch protection but not repository rulesets and also it only takes into account review state and required status checks and contains a couple of bugs, but there is an open PR to fix them.

stasostrovskyi avatar Mar 23 '24 09:03 stasostrovskyi

Indeed: https://github.com/runatlantis/atlantis/pull/4193. Thanks for the tip

dimisjim avatar Mar 25 '24 09:03 dimisjim

In GitHub there is feature that can be enabled per repo - merge queue, which allows running commands just in the middle between PR schedulled for merge and before the actual merging. We think this is the best place for applying terraform. Is there anyone who successfully configured atlantis to achieve this?

mwos-sl avatar May 06 '24 20:05 mwos-sl