-
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
fix: Do not unnecessarily update apply check if it doesn't exist yet #3747
fix: Do not unnecessarily update apply check if it doesn't exist yet #3747
Conversation
The CI failure above is I believe unrelated to my change, I addressed it separately here: #3748 |
OK now this round of CI issues are mine, I will look into them :) |
@lukemassa I'm sorry for not being familiar with the specifications of GitLab since I don't use it (In fact, this behavior posed no issues with GitHub). |
@chroju great thanks! Yeah I think github deals with these checks very differently than gitlab, so it takes some tweaking to get logic to work for both. I also updated the CI slightly in a way I think makes sense, still open to feedback there though. |
9b41e91
to
0ec2561
Compare
OK I've cleaned up CI, ready for review! |
Hi guys, I've tested this code on GitLab and GitHub, and it is working successfully. GitLab no longer has a pending @GenPage / @jamengual, can we get this reviewed and merged for the next release, as Thanks! |
…unatlantis#3747) * Do not unnecessarily create apply pipeline if it doesn't exist yet * Updates * Fix remaining * Fix test logic * Cleanup more tests * Fix test --------- Co-authored-by: PePe Amengual <[email protected]>
…unatlantis#3747) * Do not unnecessarily create apply pipeline if it doesn't exist yet * Updates * Fix remaining * Fix test logic * Cleanup more tests * Fix test --------- Co-authored-by: PePe Amengual <[email protected]>
what
When determining how to update commit status, if there are plans that still need to be applied, do not update the
apply
status at all (as opposed to now where it is updated toPendingCommitStatus
).why
#3378 introduced a change to the logic on updating plans, which includes a pre-emptive update to the "apply" check. The intention here was, for VCSs that have an apply check "in progress", to set it to "successful", to allow tools that auto merge to not have to run a full
atlantis apply
if there are no changes.In gitlab, this works fine for plans with no changes. However, the logic as is sets the apply check to "pending" if there are changes. In gitlab, this creates a new pipeline on the MR, that is then stuck in "Running" state (See #3725).
I don't know how other VCSs work, but in gitlab, calling UpdateStatus creates/updates an "external" pipeline with whatever name you give it, which makes it seem like it's "still happening" instead of just waiting for the next command. This change therefore prevents the pipeline for being stuck.
I can't imagine this PR would cause an issue to other VCSs, because this returns us, in this particular case, to the "pre 3378" world, where we didn't update the apply check at all in the plan command. Even if the apply check exists, there's no need for plan to "remind" apply that it's in progress (any more than there was a need to do so before 3378).
I could potentially see an argument for likewise returning early if
numErrored > 0
instead of updating apply with a FailedCommitStatus, but that isn't hurting anything so happy to keep it.tests
A plan with changes does not run an apply pipeline:
However a plan with no changes correctly "runs" (and successfully completes) an apply pipeline:
references
closes #3725