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

Conversation

Projects
None yet
4 participants
@terlar
Contributor

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

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
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Feb 9, 2017

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.

Contributor

krader1961 commented Feb 9, 2017

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.

terlar added some commits Feb 9, 2017

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)
@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Feb 11, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@faho

faho Feb 13, 2017

Member

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!

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Feb 14, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Feb 26, 2017

Member

The being-developed string tokenize tool might help.

Member

zanchey commented Feb 26, 2017

The being-developed string tokenize tool might help.

develop7 added a commit to develop7/fish-shell that referenced this pull request Apr 17, 2017

Add completions for helm (fish-shell#3829)
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment