Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add support for bash/zsh completion #2833

Merged
merged 1 commit into from
Feb 14, 2020
Merged

Add support for bash/zsh completion #2833

merged 1 commit into from
Feb 14, 2020

Conversation

yiannistri
Copy link
Contributor

  • Add support for bash and zsh completion
  • Currently using the builtin cobra generated scripts but can be extended to support nouns

Fixes #1593

@yiannistri yiannistri requested a review from 2opremio February 7, 2020 14:45
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Except for the minor comment this looks good to me, thanks for putting time and effort into this 🌷

I would like another maintainer (maybe @stefanprodan or @squaremo) to have a look at this, as I am not really familiar with the ins and outs of Cobra.

cmd/fluxctl/completion_cmd.go Outdated Show resolved Hide resolved
@yiannistri
Copy link
Contributor Author

@hiddeco seems like the docker container running kubeadm to set up the e2e tests run out of memory 😕Any idea if this is a transient failure and if so could we trigger the build again?

@hiddeco
Copy link
Member

hiddeco commented Feb 11, 2020

It is a transient error we are working on with the folks of Kind, I have rebased your PR to get rid of the commits from master that were accidentally pulled in, which has also triggered a new CI build 🤞

@hiddeco
Copy link
Member

hiddeco commented Feb 12, 2020

/additional-reviewer-roulette (whoever comes first)

}

return &cobra.Command{
Use: "completion shell",
Copy link
Member

@squaremo squaremo Feb 12, 2020

Choose a reason for hiding this comment

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

Do we want to distinguish syntactically between completion which is literal, and shell which is user input? (or indeed, give the possible values)

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense here to utilize the shells (again), and put <> around the list to highlight the fact that they are options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, only issue is that the Use field is also used by the root command as a way to distinguish which of the commands need to skip port forward. So the completion command would have to get instantiated there (or come up with a different way of detecting which commands are run). Alternatively, we could change it to completion SHELL to distinguish them, similar to kubectl. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Given people are hopefully already familiar with kubectl, I think that’s a good alternative.

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Solid work, thank you!

@squaremo
Copy link
Member

(needs a rebase before it can be merged; if you don't get to it @yiannistri then someone will do it eventually :-)

@hiddeco hiddeco added this to the 1.19.0 milestone Feb 12, 2020
@hiddeco
Copy link
Member

hiddeco commented Feb 14, 2020

@yiannistri can you please rebase on top of master (and maybe smash your commits together)?

@hiddeco hiddeco merged commit 05ece2d into fluxcd:master Feb 14, 2020
@hiddeco
Copy link
Member

hiddeco commented Feb 14, 2020

Thank you @yiannistri! 🌻

@yiannistri yiannistri deleted the feature/fluxctl-bash-completion branch February 18, 2020 14:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fluxctl bash completion?
3 participants