A way to block users from merging the PR/MR without first applying the changes

[AndreZiviani]

I tried to add (#2053) a pending apply status to the PR/MR in order to block the user from merging without running atlantis apply first but it ended up breaking other Atlantis use-cases (#2138 #2125) that I haven't think of at the time and a rollback was necessary (#2173), sorry for that

I will try to add an option to enable/disable this behavior (maybe --pending-apply-status? suggestions?) in order to allow both use-cases to work correctly. This is also kind of related to #2172

What do you think? @zxpower @jamengual @chenrui333 @jacekn

[jamengual]

sounds good to me, adding the option parameter will be good.

…

On Fri., Apr. 1, 2022, 12:38 p.m. Andre Ziviani, ***@***.***> wrote: I tried to add (#2053 <#2053>) a pending apply status to the PR/MR in order to block the user from merging without running atlantis apply first but it ended up breaking other Atlantis use-cases (#2138 <#2138> #2125 <#2125>) that I haven't think of at the time and a rollback was necessary (#2173 <#2173>), sorry for that I will try to add an option to enable/disable this behavior (maybe --pending-apply-status? suggestions?) in order to allow both use-cases to work correctly. This is also kind of related to #2172 <#2172> What do you think? @zxpower <https://github.com/zxpower> @jamengual <https://github.com/jamengual> @chenrui333 <https://github.com/chenrui333> — Reply to this email directly, view it on GitHub <#2176>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERGOLFEWNMOMK42FIYTVC5GBVANCNFSM5SJTRCIQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jacekn]

It sounds useful for atlantis/apply to go into pending state, maybe even by default? It will let users control required checks better. But I think it won't solve all use cases so on top of this option to apply on merge would be useful.

Imagine we want to require approval from CODEOWNERS before atlantis apply. To enforce such requirement we need to set mergable apply requirement in atlantis. To prevent humans from merging we also need to require atlantis/apply status check to pass before merging. Now we're in a situation where the PR can only go into mergable state when atlantis/apply passes but we don't allow atlantis apply until PRs are mergable.

[AndreZiviani]

That is exactly what I did but there are some use-cases that wait for all checks to be completed before proceeding so my change broke those workflows, a flag to enable this behavior should be enough

on top of this option to apply on merge would be useful.

I don't think this is possible, what should Atlantis do if the apply fail? Rollback the PR? How would you notify the user that the apply failed/succeeded?

[jacekn]

I don't think this is possible, what should Atlantis do if the apply fail? Rollback the PR? How would you notify the user that the apply failed/succeeded?

The issue with failed apply is definitely a valid concern, it would have to be handled somehow, possibly by posting an error message in the PR to notify the user.
But counter argument is that if we don't have a way to block merges before atlantis apply is run we end up in a situation where code might not match reality if people merge changes manually. Atlantis currently doesn't handle this situation at all so it's a trap that people see next time they open a PR.

So both "apply on merge" and "require atlantis apply comment" ways have downsides and can lead to drift. I believe that an option to choose apply behaviour would users choose what failure mode they prefer and it would let them build workflows around that.

FYI there is a feature request to add optional "apply on merge":
#2172

[AndreZiviani]

The issue with failed apply is definitely a valid concern, it would have to be handled somehow, possibly by posting an error message in the PR to notify the user.

Yeah but Atlantis does not accept commands on closed PR so the user would need to open another PR to fix the issue (eventually do this multiple times), I don't think this is a good solution but maybe someone have a good way to implement it

But counter argument is that if we don't have a way to block merges before atlantis apply is run we end up in a situation where code might not match reality if people merge changes manually. Atlantis currently doesn't handle this situation at all so it's a trap that people see next time they open a PR.

I agree, that's what I'm trying to improve

I've already skimmed through the code, I believe I can implement it on the following days

[AndreZiviani]

I've added an option to enable/disable this behavior, it would be really helpful if some could help review/test
#2188

[nitrocode]

You can add the atlantis/apply as a required check and then use https://www.runatlantis.io/docs/server-configuration.html#gh-allow-mergeable-bypass-apply in github. That blocks users from merging changes until atlantis/apply is green and it will become green only after a user comments atlantis apply in the PR.

[dimisjim]

@nitrocode this is currently not possible due to: #4193

[poonsalai]

Hi @dimisjim

Could we please get going for this.. As seems #4193 has been merged?

[dimisjim]

@poonsalai The PR you linked is merged and related issues have been closed. This is most likely gonna get released on the next one. I am not a maintainer on this repo, thus can't tell you when this is gonna happen.