fix(helm): No more retry if jobs failed beyond BackoffLimit #13364
Conversation
Signed-off-by: dayeguilaiye <[email protected]>
pull-request-size
bot
added
the
size/S
Signed-off-by: dayeguilaiye <[email protected]> Signed-off-by: dayeguilaiye <[email protected]>
Signed-off-by: dayeguilaiye <[email protected]>
|
@robertsirc I fixed the lint problem, could you review again? |
|
Hi @dayeguilaiye i pulled this down and ran all the unit test, and a few of my smoke test. What I think we need is a unit test around this functionality. Is there a way we can add in some test coverage? |
|
@robertsirc I found that there is no any test for But I found that there are some test for Thanks for your reviewing and advise. |
Signed-off-by: dayeguilaiye <[email protected]>
pull-request-size
bot
added
size/M
|
@robertsirc I've added it as I said. Could you review again? |
|
Also you might want to look at latest I did a bit of work in writing up more unit test for coverage. |
| @@ -91,6 +93,10 @@ func (w *waiter) isRetryableError(err error, resource *resource.Info) bool { | |||
| w.log("Status code received: %d. Retryable error? %t", statusCode, retryable) | |||
| return retryable | |||
| } | |||
| if errors.Is(err, ErrNoRetryError) { | |||
There was a problem hiding this comment.
Bit of a nit, but seems like this should be above the if statement above
| @@ -91,6 +93,10 @@ func (w *waiter) isRetryableError(err error, resource *resource.Info) bool { | |||
| w.log("Status code received: %d. Retryable error? %t", statusCode, retryable) | |||
| return retryable | |||
| } | |||
| if errors.Is(err, ErrNoRetryError) { | |||
| w.log("The error is a NoRetryError, Retryable err? %t", false) | |||
There was a problem hiding this comment.
Not much value in this log to me
| @@ -38,6 +38,8 @@ import ( | |||
| "k8s.io/apimachinery/pkg/util/wait" | |||
| ) | |||
|
|
|||
| var ErrNoRetryError = errors.New("this error will stop retry") | |||
There was a problem hiding this comment.
Maybe something along the lines of "retry limit exceeded waiting on resource"
|
I want to fix this issue, if @dayeguilaiye is not taking progress, may I make a new PR to fix it? |
What this PR does / why we need it:
This PR closes #13317
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)