s3 publisher errors should root from base error #4401
Conversation
udsamani
changed the title
udsamani
changed the title
| return err | ||
| return errors.New(err.Error()) |
There was a problem hiding this comment.
please revert this, either do:
return err
or
return fmt.Errorf("<helpful message>: %w", err)
| return fmt.Errorf("could not get job %s: %w", jobID, err) | ||
| return err |
There was a problem hiding this comment.
I assume this is being removed since the Get method's error already contains a detailed description?
| return err | ||
| return errors.New(err.Error()) |
There was a problem hiding this comment.
similar comment as stated elsewhere.
| @@ -33,7 +34,7 @@ func DownloadResultsHandler( | |||
| JobID: jobID, | |||
| }) | |||
| if err != nil { | |||
| Fatal(cmd, fmt.Errorf("could not get results for job %s: %w", jobID, err), 1) | |||
| return errors.New(err.Error()) | |||
There was a problem hiding this comment.
similar comment as stated elsewhere.
| return job, bacerrors.NewJobNotFound(jobID) | ||
| return job, apimodels.NewJobNotFound(jobID) |
There was a problem hiding this comment.
I think we can used jobstore.NewErrJobNotFound here, no?
| if resp.StatusCode == http.StatusUnauthorized { | ||
| return apimodels.NewUnauthorizedError("invalid token") | ||
| } |
There was a problem hiding this comment.
Do you think it would be cleaner to do:
return apimodels.NewAPIError(http.StatusUnauthorized, "invalid token")instead?
The methods like NewBadRequestError, NewForbiddenError, etc. feel like unnecessary wrapping.
| const ( | ||
| JOB_STORE_COMPONENT = "JBS" | ||
| BOLTDB_COMPONENT = "BDB" | ||
| ) |
There was a problem hiding this comment.
What is the different between these components? BOLTDB_COMPONENT seems like an implementation detail.
Also, similar comment here regarding naming.
| return apimodels.ErrInvalidToken | ||
| } else if err != nil { | ||
|
|
||
| _, resp, err := c.doRequest(ctx, http.MethodGet, endpoint, r) //nolint:bodyclose // this is being closed |
There was a problem hiding this comment.
remember to close the body, there is a potential for it to not get closed here if the status code isn't OK
| if apiError != nil { | ||
| return apiError | ||
| } | ||
|
|
||
| defer resp.Body.Close() |
There was a problem hiding this comment.
move this up as other comment suggests.
|
|
||
| const S3_PUBLISHER_COMPONENT = "S3PUB" | ||
|
|
||
| func NewErrBadS3Request(msg string) *models.BaseError { |
There was a problem hiding this comment.
nit on naming here and elsewhere, would prefer:
NewBadS3RequestError
and more generally:
New<Type>Error
- introduced by #4366, we need to close the body only if there isn't an error, else the body is nil and panics. Mentioned here #4401 (comment) Co-authored-by: frrist <[email protected]> Co-authored-by: Walid Baruni <[email protected]>
This PR aims at standardizing S3 Errors to use the BaseError Present