feat: improved error messages for cloud- or oss-only commands #140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danxmoran
reviewed
Jun 23, 2021
api/templates/api.mustache
Outdated
| func (r {{#structPrefix}}{{&classname}}{{/structPrefix}}Api{{operationId}}Request) OnlyCloud() {{#structPrefix}}{{&classname}}{{/structPrefix}}Api{{operationId}}Request { | ||
| r.isOnlyCloud = true | ||
| return r | ||
| } |
There was a problem hiding this comment.
I'd vote to keep things simple for now and have the OSS-vs-Cloud toggle at the API level, and not per-request
There was a problem hiding this comment.
Done! Should make it easier to keep track of what has been flagged for a specific server type for sure.
williamhbaker
force-pushed
the
wb-improved-errs-129
branch
from
38b6293 to
712edc7
Compare
danxmoran
reviewed
Jun 23, 2021
danxmoran
approved these changes
Jun 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #129
Note for future readers: As the review discussion indicates, we ended up only implementing chaining the
OnlyOSS()/OnlyCloud()methods off of API services, rather than individual requests. The original PR description is below in its entirety for reference.This PR adds the ability to set properties on individual API requests or all requests in a particular API service by chaining methods called
OnlyOSS()orOnlyCloud()when the requests or services are created. I also tweaked the generic "undefined response type" output to be a little more descriptive.If the request fails, the resulting error will be changed from something like this:
...to something like this:
As part of this, I set
BackupApiandRestoreApito be "OnlyOSS", and theGetHealthrequest of thePingcommand to be "OnlyOSS" as well. We could set the entireHealthApito be "OnlyOSS" - I did this for demonstration purposes more than anything...and there is actually a working/pingendpoint on cloud so maybe we'll want to use that someday.For cloud-only endpoints, the
BucketsSchemaApiwas set to "OnlyCloud", which changes the error message when trying to use the command on an OSS server:...becomes:
Future work, which is currently blocked by influxdata/influxdb#20224 (& the equivalent cloud issue), could explicitly check the
X-Influxdb-Versionbased on these properties and return a more specific error message. This could even include a pre-flight check of some kind to make sure the server supports the command if they are flagged as being cloud- or oss-only before trying to execute the requests. I'll create an additional issue to document this further work when we merge this PR.