-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Add option to lock on apply instead of plan #3879
Conversation
This is beautiful, thank you. Exceptionally keen for this to land. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @peikk0 Thanks for the contribution, lets get another review and then we can merge
Thanks @jamengual! 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of usability, could we possibly combine this with the existing repo_locking
that default to true
?
My thoughts here are instead of two flags with a boolean that could be confusing from a UX perspective and be conflicting. We change the repo_locking
to off|plan|apply
.
I think that makes sense but it should break existing configurations, both server-side and in repos. 🤔 |
Could use a new variable like |
we have something similar here #3895 with autoplan: mode |
e59d8c2
to
89a10fa
Compare
This could be a good compromise; Thank you, @nikolaik. Can you adjust the PR @ peikk0? |
Sounds good, I will add that. |
Just a note that I'm now working on this after a holiday break, hoping to push the update some time next week. 🤞🏻 |
@peikk0 can you look at the linter errors? |
@jamengual I've fixed the linting errors related to this change. The deprecation warning in |
@peikk0 last one, could ou fix the conflicts so we can merge it? |
@jamengual conflicts fixed 👍🏻 |
thanks for the contribution @peikk0 |
c9cab8d
to
f1569d8
Compare
@jamengual I've amended my merge commit for some stuff I reverted by mistake in the docs, sorry about that 🙇🏻 |
Are these docs necessarily up to date following this PR? https://www.runatlantis.io/docs/locking Also, now that locking can occur on_apply and parallel plans are allowed across the same project (for multiple PRs) this means there is no longer a no-op method of locking a project to a specific PR and disallowing applies without first running A way to mitigate this would be a manual Do y'all have other recommendations for this type of locking on 1 PR while also using |
Those docs are up to date with the default locking, if you use any other option then the docs are per flag in the repo config or server config. if you do |
@jamengual Do you think there's a no-op apply pattern that would work with atlantis to enable PR locks without applying changes to a root (or really a set of many many roots)? |
if understand correctly you could use draft pr to plan or a regular pr but
I don't understand why you will have to apply instead of just closing the
pr if the idea is not to apply the code.
if that is not what you mean please describe the user flow of what you are
trying to do.
…On Fri, Aug 16, 2024, 7:01 a.m. Harrison Katz ***@***.***> wrote:
there is no longer a no-op method of locking a project to a specific PR
and disallowing applies *without* first running atlantis apply for the
specific PR.
@jamengual <https://github.com/jamengual> Do you think there's a no-op
apply pattern that would work with atlantis to enable PR locks without
applying changes to a root (or really a set of many many roots)?
—
Reply to this email directly, view it on GitHub
<#3879 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ3ERGFP3GEOWVF64HQ3FDZRYA3FAVCNFSM6AAAAAA6ISPQLCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJTGU3DONZQGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
So the use-case we have had in the past is via
To make a similar process with We would ideally still want a way to "claim" the projects via an Am I making any sense? |
@jamengual Any thoughts on this use-case?^ |
is a good use case 😎 |
Great! I've opened a feature request for this: #4877 |
what
Add option
lock_repo_on_apply
to lock on apply instead of plan.why
Use case:
At GitLab we want to use Atlantis in repositories where there can be dozens of Merge Requests being worked on simultaneously, and we want an autoplan on every push, so locking on plan would be very annoying very quickly. Non-locking draftplans would also not be a complete solution, as most often those MRs are not drafts and are just waiting for a review and approval, which can be for many hours. We also have Renovate opening new MRs for automatic updates every day.
Instead, the idea here would be to be able to lock only from the first apply in a MR and until it is fully applied and closed. Plans going stale and needing to be re-planned (even after review and approval) are expected and acceptable in this scenario.
tests
lock_repo_on_apply: true
set in the server-side repo configatlantis plan
, verified in Redis that the lock wasn't setatlantis apply -p my_env
, verified in Redis that the lock was setatlantis apply
, verified in Redis that the lock was deleted after the MR was merged.Also
make test
.references
Alternative solution for #1125, #2237