Skip to content
This repository was archived by the owner on Feb 16, 2024. It is now read-only.

[Merged by Bors] - Support generation of shell completions #54

Closed
wants to merge 8 commits into from

Conversation

sbernauer
Copy link
Member

@sbernauer sbernauer commented Jul 19, 2022

Description

For #37

This was surprisingly easy. Following the same semantics as kubectl.

Docs will be added via c97f6ac in #36.
We can copy parts of kubectl docs

You can try it out with source <(stackablectl completion bash) (for bash)

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@sbernauer sbernauer requested a review from a team July 19, 2022 09:35
@siegfriedweber siegfriedweber requested review from siegfriedweber and removed request for a team July 19, 2022 13:42
@sbernauer
Copy link
Member Author

If you have any installation instructions for your shell (in addition to c97f6ac) please paste them here and i will add them to the docs.

@siegfriedweber
Copy link
Member

I tried it with the fish shell. The command and option names are auto-completed but the values are not.

I would have expected that at least the static option values can be completed like stackablectl --log-level. But there is no hint what to enter.

It would be great if also the dynamic option values would work. The auto-completion in kubectl is tremendously helpful with that:

$ kubectl logs --namespace 
default  kube-node-lease  kube-public  kube-system  local-path-storage

$ kubectl logs --namespace default
commons-operator-deployment-747bbfd76f-8wx7c  secret-operator-daemonset-sj4dn
hbase-operator-deployment-846bfb56d6-z2zdf    secret-operator-daemonset-z454t
hdfs-operator-deployment-69bdd9995c-5rv6v     zookeeper-operator-deployment-5b5b8c5896-hlwtb
secret-operator-daemonset-q5ptw

@siegfriedweber siegfriedweber requested review from a team and removed request for siegfriedweber July 21, 2022 06:40
@sbernauer
Copy link
Member Author

sbernauer commented Jul 21, 2022

Thanks for your review!
I noticed the missing --log-level completion as well, reason being its an external enum we are using. With code-duplication or some Rust magic we can fix this. I will give it a try.
The whole dynamic completion would be awesome but also pretty complicated (you can get an idea here)
I would suggest merging this with only the static functionality and look into dynamic completion once clap improves support for this.
But feel free to pick this up :)
Is this ok for you?

@soenkeliebau
Copy link
Member

The funcitonality in this PR is fine for now, we'll open a follow up ticket for the more fully fledged functionality that also completes release names etc.

@sbernauer
Copy link
Member Author

@siegfriedweber with @soenkeliebau's comments in mind: could you please review the PR again? The --log-level option should work now

Copy link
Member

@siegfriedweber siegfriedweber left a comment

Choose a reason for hiding this comment

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

The auto-completion generated by clap adds some value but depending on the used shell, the value is limited. For instance, in the fish shell the auto-completion stops working after the first option (stackablectl -l info -). The workaround is to discard the spaces on short options (stackablectl -linfo -) but this does not work for long options like --additional-release-files. However, this works in bash.

@sbernauer
Copy link
Member Author

Thx!

@sbernauer
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 28, 2022
## Description

For #37

This was surprisingly easy. Following the same semantics as `kubectl`.

Docs will be added via c97f6ac in #36.
We can copy parts of `kubectl` docs

You can try it out with `source <(stackablectl completion bash)` (for bash)
@bors
Copy link

bors bot commented Jul 28, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Support generation of shell completions [Merged by Bors] - Support generation of shell completions Jul 28, 2022
@bors bors bot closed this Jul 28, 2022
@bors bors bot deleted the shell-completion branch July 28, 2022 07:35
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.

3 participants