-
Notifications
You must be signed in to change notification settings - Fork 164
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: Ignore 422 response while creating commit statuses #299
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ragnar Paide <16119863+ragnarpa@users.noreply.github.com>
208181a
to
39deccf
Compare
@pasha-codefresh, can you review it? |
Hey-hey. @pasha-codefresh, any updates? |
Hi @pasha-codefresh, I hope you're doing well! It's been over six months since the proposed change was shared. Do you happen to see any issues with it? |
@ragnarpa Taking a look |
eventSequence.addError(fmt.Errorf("failed to deliver notification %s to %s: %v using the configuration in namespace %s", trigger, to, err, apiNamespace)) | ||
|
||
var ghErr *services.TooManyCommitStatusesError | ||
if ok := errors.As(err, &ghErr); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont really like to have github specific logic here, i think we should create some struct / util that will identify list of errors that we want to treat as warning and not as error , also skip retry. So in future we can just add another error to ignore from another provider here. WDYT?
Ignore HTTP 422 responses by GitHub while trying to create commit statuses. GitHub limits creating commit statuses to 1000 attempts with the same SHA and context. Currently, when the limit is reached, Argo CD notification controller keeps retrying needlessly which wastes resources. Instead, the notification engine should give up on retrying and signal the caller that the notification attempt has been processed.