CLOUDP-60178: Implement logs API, Atlas #79
Conversation
gssbzn
left a comment
gssbzn
left a comment
There was a problem hiding this comment.
@andreaangiolillo this is working with a quick test I did, it may need some more love so leaving it with you
| } | ||
|
|
||
| req.Header.Add("Content-Type", mediaType) | ||
| if body != nil { |
There was a problem hiding this comment.
only set the Content-Type if there's a body
| // pointed to by v, or returned as an error if an API error has occurred. If v implements the io.Writer interface, | ||
| // the raw response will be written to v, without attempting to decode it. | ||
| func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) { | ||
| resp, err := DoRequestWithClient(ctx, c.client, req) |
There was a problem hiding this comment.
I would like to get rid of DoRequestWithClient since it doe snot offer any advantage but I don't know if someone is using it
| if err != nil { | ||
| return nil, err | ||
| decErr := json.NewDecoder(resp.Body).Decode(v) | ||
| if decErr == io.EOF { |
There was a problem hiding this comment.
this is a small bug fix I did for om, bringing it here
gssbzn
left a comment
gssbzn
left a comment
There was a problem hiding this comment.
Couple of nits, mostly with adding a body payload to the gzip request, I think this can be completly avoided do to the nature of the request
gssbzn
left a comment
gssbzn
left a comment
There was a problem hiding this comment.
I don't think this solves what we agreed to do with Get and Download let mw know if you want to chat about it
gssbzn
left a comment
gssbzn
left a comment
There was a problem hiding this comment.
Couple of small suggestions but we are almost there
gssbzn
left a comment
gssbzn
left a comment
There was a problem hiding this comment.
small nitpicks with handling of errors
gssbzn
left a comment
gssbzn
left a comment
There was a problem hiding this comment.
LGTM, great work here, I know it took some effort
|
@themantissa this is ready for review, it includes some changes on how requests are created/executed to let us handle gzip request so you may want to give that some extra attention |
themantissa
left a comment
themantissa
left a comment
There was a problem hiding this comment.
This looks good from my general perspective but definitely need either Paco or Marin to check the downstream impact to the Terraform provider, Vault and CloudFormation for the work in mongodbatlas.go and mongodbatlas_test.go. May take them a bit longer to review this one so keep that in mind.
|
@PacoDw and @marinsalinas as the two most familiar with this client and all the downstream consumers ( Terraform provider, Vault and CloudFormation) can you in particular review the changes in mongodbatlas.go, and mongodbatlas_test.go, especially around the new request interface, etc? This may take a bit longer so if we need to use retainer hours that is fine. |
marinsalinas
left a comment
marinsalinas
left a comment
There was a problem hiding this comment.
I have a question, will the gzip request be only GET requests? If not we have to keep in mind to add body validation in gzip request right?
|
@marinsalinas yes, we don't have POST APIs that return gzip content |
CLOUDP-60178