Feat: Don't retry function upload on 400 or 422 status #478
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -538,10 +538,17 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl | |
| context.GetLogger(ctx).WithError(operationError).Errorf("Failed to upload file %v", f.Name) | ||
| apiErr, ok := operationError.(apierrors.Error) | ||
|
|
||
| if ok { | ||
| if apiErr.Code() == 401 { | ||
| sharedErr.mutex.Lock() | ||
| sharedErr.err = operationError | ||
| sharedErr.mutex.Unlock() | ||
| } | ||
|
|
||
| // TODO this changes retry behaviour for the fileUpload case as well. OK? | ||
| if apiErr.Code()/100 == 4 { | ||
| return nil | ||
JGAntunes marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. So I had to spend some time looking through this but I believe there's some room for improvement here (in the retry strategy I mean 😅) I was initially going to say that we didn't want to
However this case is also unideal because it just means we'll keep on retrying and erroring out here until we exhaust the count. I was reading through the docs for |
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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 would voice the same concern as @biruwon pointed out here. I'm concerned that retrying all the 4xx calls might be a stretch? (i.e. should we retry 429's too?)
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.
Updated to just skip the retry for 400 and 422 status codes 🙂