fix: In Gitlab, if an Atlantis 'Apply' Pipeline Job fails, it cannot be Re-Applied on the Same Commit

[X-Guardian]

what

Update the GitLab client PullIisMergeable function to return true if mr.DetailedMergeStatus is ci_must_pass.

The function already individually checks all the commit statuses, and will return false if any other status other than Atlantis Apply statuses do not have a status of success, so allowing this status will not stop other non-Atlantis failed CI checks from preventing an Atlantis apply.

why

Fixes #3999

tests

Tested locally and Gitlab Client unit tests updated.

[lukemassa]

I was personally unable to reproduce this issue, since for me the detailed merge status was ci_still_running during execution, which is already covered. That said, I tested this change and it doesn't seem to cause a regression, and the logic is sound. I'd still like to have a longer term conversation about the semantics of "mergeable", but in the meantime this is a step in a good direction.

[X-Guardian]

That means that you still had a pipeline running, either a Gitlab or external one.

[lukemassa]

Before and after I ran atlantis apply I checked the API and it said ci_must_pass. I assumed that the pipeline that was running was precisely the one the code was working on (i.e. "atlantis/apply"). Are you experiencing something different?

[X-Guardian]

If you have a failed atlantis apply, all other pipelines have finished and were successful and Pipelines must succeed is set on the GitLab repo, then the merge status API will return ci_must_pass in DetailedMergeStatus, which before this change would have prevented you running atlantis apply again if you have Atlantis apply_requirements: mergeable set.

[lukemassa]

Here's what I'm seeing. I have set mergeable and have only_allow_merge_if_pipeline_succeeds set to True, and then I constantly called the API in a separate browser as I went through the workflow.

1. Open MR ready to go (status: mergeable)
2. apply MR (status: ci_still_running)
3. apply fails (status: ci_must_run)
4. Replan (status: ci_still_running)
5. Plan is finished (status: ci_must_run)
6. apply (status: ci_still_running) <-- this is the point where the code in question is executed
7. apply succeeds and the MR is merged (status: not_open)

As far as I can tell, there's no way for status to be ci_must_run at this point in the code, since at the point, the ci is, by definition, running, so the API returns "ci_still_running".

Again, this is just what I'm seeing, and I have no issue with this change going in, since I think it's "more correct" than what we have now. I was just curious if this differs from what you're seeing, and potentially concerned this might not fix your problem.

[X-Guardian]

Why are you doing steps 4 and 5? That is not the scenario that this change is fixing. Just try running apply again at step 4.

[lukemassa]

I tried again without 4 and 5 and got the same result. Here's my thought process:

1. Commenting "atlantis apply", regardless of whether it has failed in the past, marks the atlantis/apply pipeline as "running"
2. The pipeline won't leave the running state to either "failed" or "success" until atlantis updates the pipeline status
3. The code in question happens between when atlantis tells gitlab the pipeline is "running" and when atlantis tells gitlab the pipeline is "failed" or "success"
4. If you hit api/v4/projects/ID/merge_requests/ID while a pipeline is in "running" state, it will return "ci_still_running", regardless of whether it returned "ci_must_pass" before.

The evidence I have for this is, when I print out the value of mr.DetailedMergeStatus during the apply, I get ci_still_running.

Two possibilities I can imagine:

1. My understanding above is wrong
2. There's a race condition, and sometimes gitlab doesn't update "ci_must_pass" to "ci_still_running" quick enough

[X-Guardian]

Are you actually running this with a real failure in the apply? If so you would see that the atlantis/apply: <dirName> pipeline(s) stay in a failed state and aren't reset by running another atlantis apply, only when the relevant pipeline actually succeeds. This is independent of the summary atlantis/apply pipeline. These failed pipelines prevent an additional atlantis apply from running in the scenario described.

[lukemassa]

I'm simulating an apply failure like this:

resource "null_resource" "alwaysfail" {
  provisioner "local-exec" {
   command = "sleep 30 && false"
  }
}

While the apply is running I see the atlantis/apply: <dirName> stay in the failed stated, but the atlantis/apply pipeline enter the "running" state. This, in turn, causes the commit status to return ci_still_running.

[X-Guardian]

How many states are you affecting in one MR? Please try more than one and remove the sleep 30.

[lukemassa]

Setting aside my inability to reproduce the issue, can you explain what's wrong about my thought process here: #4007 (comment)

For example, maybe 1) is wrong and atlantis/apply doesn't always enter a running state? Or maybe 4) is wrong and sometimes even if one of the pipelines in a commit are running the detailed status will say ci_must_run?

[X-Guardian]

Because you aren't considering the one to many directory specific atlantis apply pipelines for an MR. If any of them are failed, the second apply will fail the isMergeable check as they will still be failed at that point and therefore the MR detailed status will be ci_must_pass.

[lukemassa]

OK I think the place where we fundamentally disagree is that, I believe that, regardless of any of the directory pipelines, during execution of the atlantis apply (and hence where this code executes), atlantis/apply pipeline will be Running, and so long as ANY pipelines are running, the detailed status will be ci_still_running.

To clarify, I'm happy to get to the bottom the differences here, but in my mind it doesn't need to block this PR. Just because I don't see how we can get into a state where this is needed doesn't mean we can't, and again the logic would be more correct given this change.

[GenPage]

Thank you both for the professional and thoughtful discussion. I will merge and we can always revert in a patch release if needed.