Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: delete cluster flag completion #3166

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: delete cluster flag completion #3166

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b04d6d
feat: delete cluster flag completion
iyear Apr 13, 2023
aff690d
chore: remove name flag constant
iyear May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 19 additions & 1 deletion pkg/cmd/kind/delete/cluster/deletecluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,14 @@ func NewCommand(logger log.Logger, streams cmd.IOStreams) *cobra.Command {
return deleteCluster(logger, flags)
},
}

const (
nameFlag = "name"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think this is more descriptive that "name" which is only used in two places anyhow ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Done

)

cmd.Flags().StringVarP(
&flags.Name,
"name",
nameFlag,
"n",
cluster.DefaultName,
"the cluster name",
Expand All @@ -61,6 +66,19 @@ func NewCommand(logger log.Logger, streams cmd.IOStreams) *cobra.Command {
"",
"sets kubeconfig path instead of $KUBECONFIG or $HOME/.kube/config",
)

cobra.CheckErr(cmd.RegisterFlagCompletionFunc(nameFlag, func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
clusters, err := cluster.NewProvider(
cluster.ProviderWithLogger(logger),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when podman support is stable and we move from KIND_EXPERIMENTAL_PROVIDER to KIND_PROVIDER as a default for a --provider flag? Can flag completion check for other flag values?

This feature seems like it may unfortunately be at least as problematic to maintain as it is useful.

Using KIND_CLUSTER_NAME is another option to get around repeatedly typing these.

Copy link
Author

@iyear iyear Apr 18, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when podman support is stable and we move from KIND_EXPERIMENTAL_PROVIDER to KIND_PROVIDER as a default for a --provider flag?

Does this mean that the implementation of kind get clusters will change? The code in PR is copied from get clusters

Can flag completion check for other flag values?

Yes it can.

or extract get clusters as a common function, and we pass in the existing args and flags then get results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the implementation of kind get clusters will change? The code in PR is copied from get clusters

Yes. It's defaulting based on availability or the env currently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how about extract kind get clusters runE as a common function ? IMO, the completion is like an alias of get clusters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've just never used a tool where one flag completed based on another flag and didn't know if cobra can support this. If it can then let's go ahead :-)

runtime.GetDefault(logger),
).List()
if err != nil {
return nil, cobra.ShellCompDirectiveError
}

return clusters, cobra.ShellCompDirectiveNoFileComp
}))

return cmd
}

Expand Down