atlantis
atlantis copied to clipboard
feat: plan queue functionality
Relates to: https://github.com/runatlantis/atlantis/discussions/1722
Hey @nishkrishnan, based on above discussion, we are submitting this PR that provides plan queue functionality.
The implementation of the queue functionality is complete and the code is almost production-ready. We are seeking your assistance and would appreciate your feedback if this is the right direction and if this functionality could be considered.
We we have done so far:
- Automatically entering the queue upon a plan request if another PR has the lock.
- Granting the lock to the next PR in queue when the previous PR is unlocked (either via explicit
atlantis unlock
command, from Atlantis UI or by merging/closing the PR). - Notifying the author that the PR is ready to be planned for the unlocked environments/workspaces.
- We avoided automatic planning because: 1) A PR can have more than one lock and not all of them might be released yet. 2) It was cumbersome to store the initial command in case extra arguments were passed.
- Showing the queue in the main UI and in the locks view.
What is missing:
- Feature flag to enable/disable the queue.
- Handling plan errors in respect to the queue: currently the lock is released on plan error, which means we should either:
- Grant the lock to the next person in the queue
- Or change that behavior and not release the lock on plan errors
- More commands to handle the queue: e.g. jump in front of the queue, or get out of the queue.
- More test coverage.
Contributors
@monikma @amir-elgayed @ghaiszaher
how this behaves on 1 PR, 5 different projects changes all w/default workspace ? would it be able to use ProjectName to produce parallel plans ? Mostly because the existing locking mechanism is tied to $workDir and how it is formulated repo+pr+workspace+path
how this behaves on 1 PR, 5 different projects changes all w/default workspace ? would it be able to use ProjectName to produce parallel plans ? Mostly because the existing locking mechanism is tied to $workDir and how it is formulated repo+pr+workspace+path
@xavipanda The key for the queue is the same as the key for the lock (see here).
As soon as one of those projects gets unlocked, a comment will be posted on the PR stating the projects for which locks are free. The PR owner has to then replan manually.
Same happens with 1 PR and 1 project & workspace. We were not sure how and if we want to plan automatically because of that multi-project/workspace situation (as mentioned in the PR description). Suggestions are welcome.
This also relates to #305 but is technically different.
@ghaiszaher @monikma Great job
This also relates to #305 but is technically different.
@majormoses As far as I understood, it is quiet different: This is about providing a queueing mechanism for plans cross different PRs that affect the same workspace (they compete for the same lock), if a lock is acquired by PR#1
, plans from other PRs will enter a queue and will be notified when the lock is released by PR#1
, whereas #305 is about plans on the same PR that is already holding the lock.
@ghaiszaher is there still interest in getting this merged?
@ghaiszaher is there still interest in getting this merged?
@jamengual yes we are still interested, we'd appreciate any feedback on the current PR.
@ghaiszaher do you have any time to address the comments?
@ghaiszaher do you have any time to address the comments?
@jamengual I tried to answer the comments, since Ghais is currently on vacation. First of all, @lilincmu thanks a lot for the review! Hopefully we can shorten the feedback cycle now, and make each next interaction exponentially quicker ;)
One more nit from me is, it might be more readable if we define []models.ProjectLock to be a new type, such as ProjectLockQueue. When reading the code, sometimes I had to figure out whether []models.ProjectLock refers to a queue, or simply a list of locks, for example, that are just being unlocked by a PR.
Makes a lot of sense to me.
About your questions on the tests - yes we have not implemented all of them, as we first wanted to check with you if this is some functionality that sounds like a good idea, and if how we go about implementing it would make sense to you (as mentioned in the PR description). So my understanding is "yes it does" ? :)
We could continue by applying the suggestions and extending tests. Would that make sense to you? What do you expect from us to call our PR "production ready", and get closer to merging it?
One topic we circled around but never properly got to, is feature flag to enable/disable the feature. If you had any suggestions there, it would be great (meaning how feature flags are usually approached in this codebase, we would not want to break some convention).
@lilincmu @jamengual Since this PR was closed because the repo was deleted from the organization, I raised a new PR: https://github.com/runatlantis/atlantis/pull/2782 which is in a better state now
cc @monikma @amir-elgayed
Except for few nits, this looks great to me! Thank you all for your contribution! 🎉
One more nit from me is, it might be more readable if we define
[]models.ProjectLock
to be a new type, such asProjectLockQueue
. When reading the code, sometimes I had to figure out whether[]models.ProjectLock
refers to a queue, or simply a list of locks, for example, that are just being unlocked by a PR.
Thank you @lilincmu, renamed as part of this commit: https://github.com/runatlantis/atlantis/pull/2782/commits/06a83e3570eae401f2913dd8b7893919e271e1c2