-
Notifications
You must be signed in to change notification settings - Fork 88
CLOUDP-325300: [AtlasCLI] Support preview and upcoming stability levels for L1 commands #3984
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
…en executing a preview version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, comments are not a blocker
internal/cli/api/api.go
Outdated
// If the version is valid, check if it's supported | ||
if err == nil { | ||
for _, commandVersion := range apiCommand.Versions { | ||
if commandVersion.Version.Equal(version) { | ||
return | ||
} | ||
} | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to return the error if the ParseVersion failed or at least log the error to stderr to help debugging.
// If the version is valid, check if it's supported | |
if err == nil { | |
for _, commandVersion := range apiCommand.Versions { | |
if commandVersion.Version.Equal(version) { | |
return | |
} | |
} | |
return | |
// If the version is valid, check if it's supported | |
if err != nil { | |
return err | |
} | |
for _, commandVersion := range apiCommand.Versions { | |
if commandVersion.Version.Equal(version) { | |
return | |
} | |
} | |
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 466 covers this message.
Example:
❯ ./bin/atlas api clusters listClusters --version not-valid
warning: version 'not-valid' is not supported for this endpoint, using default API version '2024-08-05'; consider pinning a version to ensure consisentcy when updating the CLI
{"links":[{"href":"https://cloud-qa.mongodb.com/api/atlas/v2/groups/67d048784f073c51c7d330a4/clusters?pageNum=1&itemsPerPage=100","rel":"self"}],"results":[],"totalCount":0}
internal/cli/api/api.go
Outdated
if err != nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above comment, we should at least log the error to stderror in case the impossible happen and we need to debug the issue. If you don't agree, you could then remove this if and have version, _ := shared_api.ParseVersion(*versionString)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I've updated these branches with an error message!
if commandVersion.PublicPreview { | ||
fmt.Fprintf(os.Stderr, "warning: you've selected a public preview version of the endpoint, this version is subject to breaking changes.\n") | ||
} else { | ||
fmt.Fprintf(os.Stderr, "warning: you've selected a private preview version of the endpoint, this version might not be available for your account and is subject to breaking changes.\n") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super clear messages 👍
// ```yaml | ||
// x-xgen-preview: | ||
// | ||
// name: charts-dashboards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
is set only for private preview since all the public preview API are in the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, removed the name
from the example yaml
…String, to match Stringer interface
Coverage Report 📈
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Proposed changes
api.Version
instead of a plainstring
atlas api
layers to supportupcoming
andpreview
(private/public) stability levelspreview
version is selectedupcoming
/preview
headers are set correctlyJira ticket: CLOUDP-325300
Testing
Manual tests
I included the
getOpenApiInfo
operation manually for testing and then ran the commands below.The operation got reverted afterwards.
Stable
Preview
Note that the CLI is requesting
preview
but the server is responding withupcoming
, this is normal server behavior because thepreview
version is deprecated.Upcoming
Server responded with an error, which is ok.
The CLI set the headers correctly.