-
Notifications
You must be signed in to change notification settings - Fork 308
Add dynamic shell completion for unmanaged-cluster #4871
Add dynamic shell completion for unmanaged-cluster #4871
Conversation
Related to vmware-tanzu/tanzu-framework#701 Signed-off-by: Marc Khouzam <kmarc@vmware.com>
I like the idea of keeping it with the code so there's less chance it will be overlooked. No strong feelings either way though, so I'm good with whatever the team decides. Will check this out tomorrow. Thanks! |
|
||
const clusterAnyStatus = "" | ||
|
||
func Setup(rootCmd *cobra.Command) { |
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.
This public API entrypoint needs a comment
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.
It also seems that this assumes that DeleteCmd
, StartCmd
, and StopCmd
have been initialized ontop of the root command. Might be worth mentioning that in a comment
return nil, cobra.ShellCompDirectiveNoFileComp | ||
} | ||
|
||
log := logger.NewLogger(false, 0) // No need for logs |
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: as far as code style, I don't think we usually do comments after a line
conf := &config.UnmanagedClusterConfig{ | ||
Provider: c.Provider, | ||
} | ||
clusterManager := cluster.NewClusterManager(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.
This could take quite some time; for example, with 3 minikube based clusters, we'd essentially be making 3 calls out to minikube profile
to load up all the clusters info for the manager. Even with one default cluster in my testing, tabbing took a good 1-2 seconds
Is it necessary that we introspect the status of a cluster? In my opinion, for completions, it's enough that that tanzu.List
returns a list of clusters it knows about on disk. I don't think we need to load up the manager for each cluster in order to only return valid clusters. For example, it'd still be valid to return a cluster that has status Unknown
that failed to provision when doing unmanaged-cluster rm
Also no strong feelings, but I think I'd also prefer it to stay next to the command initialization code. It'd be easy to just call |
This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days. |
This PR was closed because it has been stalled for 30 days with no activity. If this PR is still relevant, please re-open a new PR against main. |
What this PR does / why we need it
This adds dynamic shell completion of cluster names for the different commands of unmanaged-cluster.
It also disables file completion for the cases where file names are not accepted.
Dynamic completion for flag values is not handled yet.
Which issue(s) this PR fixes
Related to vmware-tanzu/tanzu-framework#701
Describe testing done for PR
To test you should have unmanaged clusters available:
Setup completion for zsh:
or setup completion for bash:
Now run some completion tests
Special notes for your reviewer
New package
I've created a new package
completion
and completely isolated the completion code in it. This is the approach that was requested forkubectl
. Forhelm
however, I chose to keep the completion code in the same file as the rest of the command (e.g., in configure.go for the configure command). The advantage of keeping the completion with the rest of the code is that I find it reduces the risk of forgetting to update it when something changes. What do we want for TCE?Completion descriptions
Cobra allows to add descriptions to each completion. It aims to help the user make a decision more easily.
Choosing a good description often requires some experience with the product so I would particularly like your input.
I chose to add the status and provider, but it may not be useful, or if it is, we could revisit the output format. I currently don't like the format I chose:
zsh:
cluster1 -- status: Running - provider: kind
bash:
cluster1 (status: Running - provider: kind)