chore: Refactor retries in gitlab set status #5293
Open
+18
−18
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what
Refactor the way retries work in
UpdateStatus
in gitlab.why
I noticed this while working on #5269; we can't prove that by the time we get to "return err" we'll actually have a non-nil error to wrap. In practice this won't happen, if you carefully follow the paths (and note that
StatusConflict
will cause err to be non-nil), but it's hard to see. Additionally there were comments like "if we get here, then..." and "return the error, which might be nil", which to me is a code smell that the logic is hard to follow.Instead I refactored following the example of the retry library we were using https://pkg.go.dev/github.com/jpillora/backoff
Another oddity that took me a bit to understand was that, whether there was a conflict or not, we retry if there's an error and we haven't hit our max amount. This means that the conflict really just amounts to a change in logging, which the new code reflects.
tests
The unit tests have good coverage of this area of the code.
references