-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stub out the configuration command and return an error #527
Conversation
cmd/up/main.go
Outdated
@@ -105,7 +104,7 @@ type cli struct { | |||
Space space.Cmd `cmd:"" help:"Interact with Spaces."` | |||
Group group.Cmd `cmd:"" help:"Interact with groups inside Spaces."` | |||
ControlPlane controlplane.Cmd `cmd:"" name:"controlplane" aliases:"ctp" help:"Interact with control planes in the current context, both in the cloud and in a local Space."` | |||
Configuration configuration.Cmd `cmd:"" name:"configuration" aliases:"cfg" help:"Interact with configurations."` | |||
Configuration configuration.Cmd `cmd:"" name:"configuration" aliases:"cfg" help:"Interact with configurations (deprecated)."` |
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 used Deprecated:
as a prefix for the help in up ctp
.
https://github.com/upbound/up/blob/main/cmd/up/controlplane/controlplane.go#L116-L117
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.
👍 Updated to match.
|
||
Flags upbound.Flags `embed:""` | ||
func (c *Cmd) Run(ctx context.Context, p pterm.TextPrinter) error { | ||
return errors.New("Configurations are not supported in Upbound Spaces") |
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.
nit: Just Upbound
not Upbound Spaces
. I can share a link to you about how we're changing the way we talk about the product, including naming schemes.
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, makes sense. Feels a bit confusing to just say "not supported in Upbound", since it used to be supported, so I've updated it to no longer supported in Upbound
to try and make it clearer why the command still exists. Open to suggestions.
The configuration command doesn't work with Upbound Cloud Spaces or Connected Spaces, and the CLI no longer works with Legacy Spaces, so remove the subcommands from `up configuration` and print a message. Fixes #493 Signed-off-by: Adam Wolfe Gordon <[email protected]>
Description of your changes
The configuration command doesn't work with Upbound Cloud Spaces or Connected Spaces, and the CLI no longer works with Legacy Spaces, so remove the subcommands from
up configuration
and print a message.Fixes #493
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
Ran the unit tests, and manually verified that
up configuration
prints the expected message.