-
Notifications
You must be signed in to change notification settings - Fork 42
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
Make ctp list derive context from kubeconfig #457
Conversation
cmd/up/controlplane/list.go
Outdated
po := clientcmd.NewDefaultPathOptions() | ||
conf, err := po.GetStartingConfig() | ||
if err != nil { | ||
return err | ||
} | ||
profiles, err := upCtx.Cfg.GetUpboundProfiles() | ||
if err != nil { | ||
return err | ||
} | ||
if err != nil { | ||
return err | ||
} | ||
|
||
_, curprof, ctp, err := profile.FromKubeconfig(ctx, profiles, conf) |
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.
Dieser ganze Block wird häufig auftreten. Wir sollten einen Helper dazu erstellen:
profileName, profile, ctp, restConfig, err := upCtx.Cfg.GetCurrentContext(ctx)
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.
English translation:
This whole block will occur frequently. We should create a helper for this:
internal/config/config.go
Outdated
@@ -122,6 +125,26 @@ func (c *Config) GetUpboundProfile(name string) (profile.Profile, error) { | |||
return p, nil | |||
} | |||
|
|||
// GetCurrentContext returns the current context from the kubeconfig, profile and config | |||
func (c *Config) GetCurrentContext(ctx context.Context) (string, *profile.Profile, types.NamespacedName, error) { |
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.
add names to the return values. It's unclear what the string is.
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.
also to FromKubeconfig please. I also forgot that.
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.
added return named values to both functions
Signed-off-by: Piotr Zaniewski <piotr@upbound.io> Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
c.client = cloud.New(ctpclient, cfgclient, upCtx.Account) | ||
} | ||
|
||
func (c *listCmd) AfterApply(kongCtx *kong.Context) error { | ||
kongCtx.Bind(pterm.DefaultTable.WithWriter(kongCtx.Stdout).WithSeparator(" ")) |
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.
Should we check for mutual exclusivity with -g
and -A
?
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.
In kube -A
trumps over -n
afaik. No strong opinion whether we are stricter here.
Description of your changes
This PR makes
up ctp list
kubeconfig context aware, making it independent from the current profile. The profile is automatically derived:$ kubectl ctx not-space-kube
$ up ctp list
You are not on a space.
$ kubectl ctx space
$ up ctp list
.... controlplanes from the profile matching the kubeconfig context's URL ....
$ up ctp connect ctp
$ up ctp list
In a controlplane, you cannot list controlplanes.
Use 'up ctx ..' to go to group level.
Fixes https://github.com/upbound/spaces/issues/752
NOTE: this breaks the use against a cloud profile. We will bring it back in a follow-up.
I have:
make reviewable
to ensure this PR is ready for review.Addedbackport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
This can be tested locally by running the following setup: