Fix missed error check in API call #54
Conversation
Also prevent the Broker from functioning when there's several broker-state apps in maintenance project
|
Steps to repair the affected environments:
|
|
Good find. |
| realmClient, err := mongodbrealm.New( | ||
| context.TODO(), |
There was a problem hiding this comment.
[q] I see that context parameter is not used in New function - is this intentional?
There was a problem hiding this comment.
Good catch - probably left over from an older version
| nil, | ||
| mongodbrealm.SetBaseURL(realmURL), | ||
| mongodbrealm.SetAPIAuth(context.TODO(), key.PublicKey, key.PrivateKey), | ||
| mongodbrealm.SetAPIAuth(ctx, key.PublicKey, key.PrivateKey), |
There was a problem hiding this comment.
correct me if I'm wrong but the context parameter which is passed everywhere is not used? mongodbrealm.NewRequest doesn't use the parameter passed. Is it planned for future?
There was a problem hiding this comment.
I'm adding context everywhere (except mongodbrealm.New above) right now - not sure if I should do it in this PR or a separate one, as it's a big change and not necessarily related to this error
There was a problem hiding this comment.
This PR looks good - maybe in the future would be great to separate different changes to separate PRs (context and error handling)
The main concern is using errors.Wrap everywhere - maybe not an issue depending on the way the final error is handled and logged
| @@ -254,16 +267,18 @@ func encodePlan(v dynamicplans.Plan) (string, error) { | |||
| b64 := base64.NewEncoder(base64.StdEncoding, b) | |||
| err := json.NewEncoder(b64).Encode(v) | |||
| if err != nil { | |||
| return "", err | |||
| return "", errors.Wrap(err, "cannot marshal plan") | |||
There was a problem hiding this comment.
interesting, wasn't aware of changes inerrors regarding the wrapping.
Am I right that errors.Wrap will override the string message so the initial message will be lost (though the causing error will be saved internally). So loggers may miss that?
Maybe errors.Errorf("..%w") would be a better way?
There was a problem hiding this comment.
This Wrap() will actually append the inner message when Error() is called: https://godoc.org/github.com/pkg/errors#Wrap
There was a problem hiding this comment.
A real-life example of this would be the following error message:
Service broker error: cannot find plan for instance "c20bdcea-6bf1-4804-ab06-bed618054e7d": cannot fetch instance: cannot find instance in maintenance DB(s): no instances found
Here every : is basically a Wrap() call, going all the way to the top.
Could be the cause of #51 & #53 in case of a flaky timeout/API issue that is not caught in time causing state to become fragmented