Skip to content
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: implement backoff + retry when GitLab SetCommitStatus returns 409 #4503

Merged
merged 3 commits into from
May 9, 2024

Conversation

jippi
Copy link
Contributor

@jippi jippi commented May 3, 2024

what

GitLab returns a 409 Conflict status when the commit pipeline status is being changed/locked by another request, which is likely to happen if you use [--parallel-pool-size > 1] and [parallel-plan|apply].

The likelihood of this happening is increased when the number of parallel apply jobs is increased.

why

Returning the [err] without retrying will permanently leave the GitLab commit status in a "running" state, which would prevent Atlantis from merging the merge request on [apply]. GitLab does not allow merge requests to be merged when the pipeline status is "running."

tests

We've been running these changes in our environment for months and it has eliminated this type of error entirely

@jippi jippi requested review from a team as code owners May 3, 2024 10:40
@jippi jippi requested review from chenrui333, lukemassa and X-Guardian and removed request for a team May 3, 2024 10:40
@github-actions github-actions bot added dependencies PRs that update a dependency file go Pull requests that update Go code provider/gitlab labels May 3, 2024
@jippi jippi force-pushed the retry-gitlab-409-errors branch from ec262b2 to 689aaf4 Compare May 3, 2024 10:40
@jamengual
Copy link
Contributor

@lukemassa since you are a Gitlab user.

@X-Guardian
Copy link
Contributor

Can you add a test for this?

@jippi
Copy link
Contributor Author

jippi commented May 4, 2024

@X-Guardian I can certainly give it a try again; I came up short figuring out how to make it work in my original implementation. Do Atlantis provide docs on how testing should be done, and the libraries being used?

@X-Guardian
Copy link
Contributor

The tests for this change need adding to the gitlab_client_test.go file. If you look at the patterns used in that, you should be able to add a new test for this scenario following the same patterns.

@GenPage GenPage added waiting-on-response Waiting for a response from the user needs tests Change requires tests labels May 4, 2024
lukemassa
lukemassa previously approved these changes May 6, 2024
Copy link
Contributor

@lukemassa lukemassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about the tests, otherwise this looks good to me!

@lukemassa lukemassa dismissed their stale review May 6, 2024 13:52

Needs Tests

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, would be good to also have some tests.

@chenrui333
Copy link
Member

lgtm, would be good to also have some tests.

I guess, I just echo what my fellow maintainers said :)

@jippi
Copy link
Contributor Author

jippi commented May 7, 2024

I have a short work week (just 2 days this week); so will probably not get it done this week - but will prioritize it next week , regardless of this is merged in with the future promise of tests or not :)

@chenrui333
Copy link
Member

yeah, that works. thanks @jippi!

@chenrui333 chenrui333 merged commit e766e14 into runatlantis:main May 9, 2024
24 checks passed
@chenrui333 chenrui333 added needs tests Change requires tests and removed waiting-on-response Waiting for a response from the user needs tests Change requires tests labels May 9, 2024
@jippi jippi deleted the retry-gitlab-409-errors branch May 9, 2024 23:53
@jippi
Copy link
Contributor Author

jippi commented May 14, 2024

The promised tests are now up at #4503 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies PRs that update a dependency file go Pull requests that update Go code needs tests Change requires tests provider/gitlab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants