Condense CatchV2ErrorDetailWithResponse and CatchV2ErrorMessageWithResponse into one#1406
Conversation
Brian Strauch (brianstrauch)
left a comment
Brian Strauch (brianstrauch)
left a comment
There was a problem hiding this comment.
Thank you!! Can we also rename responseBody --> errorResponseBody?
| if ok, _ := regexp.MatchString(quotaExceededRegex, detail); ok { | ||
| return NewWrapErrorWithSuggestions(err, detail, QuotaExceededSuggestions) |
There was a problem hiding this comment.
Not a huge fan that this is hardcoded here... this only applies to one service so it's wasteful to always check for it
There was a problem hiding this comment.
Which service? There are a few other specialized catchers in this file, maybe we can make a specialized one for this.
There was a problem hiding this comment.
Yeah... this probably needs an updating. I just took a look at some of the quotas by listing them on the CLI. Tried to create 11 environments and hit a quote error that matches quotaExceededRegex, but didn't get the suggestion.
So this hard-coding only prints that suggestion on whatever subset of commands uses (what is now) CatchCCloudV2Error, which is sub-optimal.
There was a problem hiding this comment.
yeah, it would be great if we could only wrap the quota service functions with this catcher
There was a problem hiding this comment.
this might be deserving of a separate pr
There was a problem hiding this comment.
I can make that my next cleanup task. I'm assuming only create and update subcommands can hit these quota errors.
There was a problem hiding this comment.
We might introduce a new subcommand that also returns that error, so in my opinion the safest thing to do here would be to wrap every function in service_quota.go with a function that iterates through a list of error catchers and applies them one by one. As of right now, that list would just contain errors.CatchCCloudV2Error() and this one.
There was a problem hiding this comment.
New v2 requests to create a resource could result in this (429 if i remember correctly?). Environment won't have this because it was still using v1 to create.
There was a problem hiding this comment.
Ah ok, I didn't realize environment was using v1.
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
|
Steven Gagniere (@sgagniere) Feel free to request a review every time you make changes! I forgot to re-review this one... |
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Co-authored-by: Brian Strauch <bstrauch@confluent.io>
3 participants
Checklist
What
Two V2 error catching functions only differed on whether they read the details or message fields, so they've been combined with the detail being preferred, and the message being used if no details are present.
References
n/a
Test & Review
ran all tests without error