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.

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

Add completions for helm #3829

Merged
merged 3 commits into from
Feb 13, 2017
Merged

Add completions for helm #3829

merged 3 commits into from
Feb 13, 2017

Conversation

terlar
Copy link
Contributor

@terlar terlar commented Feb 8, 2017

This adds completions for helm. I have worked quite a lot on completions
recently and I am starting to find a pattern that seems to work for most
things. I have extensive ones for kubectl that I am working on. Perhaps
we can incorporate some of these methods and share to make it easier for
people to create completions.

About the useful changes:

  • Using command works in any position and for several
    occurrences. Sometimes you want to use a global switch in front of the
    first command, that does not work with the usual
    ..._using_command. This also ensures that you are using subcommands
    with options inbetween, but making sure they come in the correct
    order.
  • Using option to check if an option is used.
  • Using option with a certain value is used.
  • I used a pattern for completing subcommands (and root level
    command). To make sure you don't get the completions again after
    having used them once. A common problem is when using just
    __fish_seen_subcommand is that you will get that same completion
    infinite amount of times.

Please let me know what you think and if you have any feedback.

About helm:

helm - is a tool for managing Kubernetes charts. Charts are packages of
pre-configured Kubernetes resources.

See: https://github.com/kubernetes/helm

helm - is a tool for managing Kubernetes charts. Charts are packages of
pre-configured Kubernetes resources.

See: https://github.com/kubernetes/helm
@krader1961
Copy link
Contributor

I only skimmed the code and it looks okay from a style and correctness standpoint. I did not review it from a "is this the way we want to do things" viewpoint. I'm going to leave that to @faho or someone else.

After some feedback from the community it seems it is good to include
the chart in the release description. This adds the chart information to
the description. So to say this is `Release of CHART`.
- Utilize complete -f, -r and -x properly
- Add some more context aware completions (chart versions, kubectl context and namespaces)
@terlar
Copy link
Contributor Author

terlar commented Feb 11, 2017

I read up a bit more on the completions and found out that some of the things I suggested could be solved by utilizing complete -a and -x to add semantic completion for options with values. I revised all my usage of -f and substituted where possible. There still might be uses for something like a function checking for used option value. As some other options might depend on another option. Checking if a regular option is used could just utilize the __fish_seen_subcommand_from.

@faho faho added this to the 2.6.0 milestone Feb 12, 2017
@faho
Copy link
Member

faho commented Feb 13, 2017

I'm not a massive fan of depending on awk for anything, but I can see that builtins are a bit painful here, so I'm gonna merge this. Thanks!

@faho faho merged commit 22a2098 into fish-shell:master Feb 13, 2017
@terlar
Copy link
Contributor Author

terlar commented Feb 14, 2017

I agree, but as you say, otherwise I would have to combine many different commands and it wouldn't be as clean, some of the things I guess could be replaced by a string replace but would be harder to understand what is going on. Perhaps a builtin for handling outputting "positional" arguments could be a thing. I could update it to use:

helm ls ^/dev/null | awk 'NR >= 2 { print $1"\tRelease of "$NF  }'
helm ls | awk 'NR >= 2 { print $1" "$2 }'
helm repo list | awk 'NR >= 2 { print $1 }'
helm search | awk 'NR >= 2 && !/^local\// { print $1 }'
helm search -l | awk 'NR >= 2 { print $1" "$2 }'

# vs
helm ls ^/dev/null | tail -n +2 | string replace -r '^(\S+)\s+(\S+\s+)+(\S+)' '$1\tRelease of $3'
helm ls | tail -n +2 | string replace -r '^(\S+)\s+(\S+).*$' '$1 $2'
helm repo list | tail -n +2 | string match -r '^\S+'
helm search | tail -n +2 | string match -r '^(?!local/)\S+'
helm search -l | tail -n +2 | string replace -r '^(\S+)\s+(\S+).*$' '$1 $2'

What do you think? Perhaps it is worth it going built-in?

Thanks for the feedback, will keep in mind for further PR:s.

@zanchey
Copy link
Member

zanchey commented Feb 26, 2017

The being-developed string tokenize tool might help.

develop7 pushed a commit to develop7/fish-shell that referenced this pull request Apr 17, 2017
* Add completions for helm

helm - is a tool for managing Kubernetes charts. Charts are packages of
pre-configured Kubernetes resources.

See: https://github.com/kubernetes/helm

* Improve helm release completions description

After some feedback from the community it seems it is good to include
the chart in the release description. This adds the chart information to
the description. So to say this is `Release of CHART`.

* Further improvements to helm completions

- Utilize complete -f, -r and -x properly
- Add some more context aware completions (chart versions, kubectl context and namespaces)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants