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

Dynamic completion for subcommands that apply to releases #5562

Merged

Conversation

marckhouzam
Copy link
Member

@marckhouzam marckhouzam commented Apr 6, 2019

What this PR does / why we need it:

Add dynamic completion after

helm status <TAB>
helm delete <TAB>
helm test <TAB>
helm history <TAB>

by fetching the list of releases from the cluster.

Special notes for your reviewer:

This PR puts in the foundation for dynamic completion support in Helm.
It is inspired by the dynamic completion provided by kubectl.
Both bash and zsh are handled.

As dynamic completion actually sends queries to the cluster, this PR handles the use of Helm flags that affect which cluster should be contacted:

--kubeconfig 
--kube-context 
--host 
--tiller-namespace

Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
Flags sometimes can be used with an = sign, such as --kube-context=prod.
In this case, the variable ${flagname} retains the = sign as part of the
flag name. However, in zsh completion, an = sign cannot be part of an
index of the associative array 'flaghash' or else it causes an error.

This commits strips the = sign out when using ${flagname} as an index.

Note that this is not a big deal since flaghash is not actually used
anywhere in Helm completion. I believe it is made available by the
Cobra framework in case some completions choose to use it.

Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
@helm-bot helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 6, 2019
Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
@marckhouzam marckhouzam changed the title Feature/improve status delete completion Dynamic completion for subcommands: status, delete, history and test Apr 7, 2019
Copy link
Member

@technosophos technosophos left a comment

Choose a reason for hiding this comment

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

LGTM. And a huge thanks for fixing that all up!

@technosophos
Copy link
Member

Also, if you'd like to file a second PR against the dev-v3 branch, we'll make sure to get this into Helm 3 as well. You can CC me on that PR

@marckhouzam
Copy link
Member Author

Sorry for the delay, somehow I never got the notification you had replied!
I will making a PR for dev-v3 soon.

@marckhouzam
Copy link
Member Author

@technosophos I was thinking that when doing completion, the list of possible releases should be obtained using 'helm list -a'. Currently this PR does not use the -a' flag. Unless you disagree I will update the PR to use the -a' flag.

Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
@marckhouzam
Copy link
Member Author

Ok, this one is ready for the master branch. I've just added one commit that uses helm list -a -q (extra -a flag) when fetching the list of releases for completion.

I'm working on a separate PR for dev-v3.

One question: is there a way to list release that start with a specified prefix? Such as helm list --prefix prom would list all releases starting with prom?

I'm asking for two reasons
1- it may speed up completion to avoid having to fetch the list of all releases but instead to focus on the ones the user is interested in,
2- from what I understand, helm will list up to 256 releases by default. So, on a cluster of more than 256 releases, this PR will cause the completion to only offer the first 256 releases to e.g., helm status <TAB>. If the user has typed helm status prom<TAB>, the situation would not be improved as the helm list behind the scene would still fetch the first 256 releases which could completely exclude the releases starting with prom. If we could specify the prom prefix in helm list, then we would focus the completion on the releases the user cares about.
Note that I didn't want to use the --max flag to fetch more than 256 releases as it may slow down completion too much; besides what should the --max value be in this case anyway?

Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
@marckhouzam marckhouzam changed the title Dynamic completion for subcommands: status, delete, history and test Dynamic completion for subcommands that apply to releases Apr 28, 2019
@marckhouzam
Copy link
Member Author

While working on the version for dev-v3, I've realized I had forgotten to include helm rollback, helm upgrade and helm get * to the list of commands that should get dynamic completion of release names.

This latest commit does this.

@marckhouzam
Copy link
Member Author

marckhouzam commented May 18, 2019

I realized that helm list already had a [FILTER] option. This commit uses it so that when doing dynamic completion of a release name, we only fetch the releases that start with what the user may have already typed.

The real value of this change is when there are more than 256 releases to list. By using the filter, we greatly reduce the chance of the release the user wants being beyond the 256 limit.

@marckhouzam
Copy link
Member Author

I haven't forgotten about adding this to dev-v3. The issue is that dev-v3 needs a newer version of Cobra for things to work properly. I've reached out to the Cobra project to create a new release, which they've agreed to do. Once it is ready, I'll be able to post this feature for dev-v3.

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

LGTM! @marckhouzam do you need to bump cobra to 0.0.4 here as well, or is this good to merge? Let me know :)

This reverts commit d2ab3f5.
The reverted commit turned out to be a workaroud another problem.
The real fix is submitted in PR helm#5680

Signed-off-by: Marc Khouzam <marc.khouzam@ville.montreal.qc.ca>
@marckhouzam marckhouzam force-pushed the feature/improveStatusDeleteCompletion branch from 3f74a06 to 1bd6281 Compare June 5, 2019 20:11
@marckhouzam
Copy link
Member Author

@bacongobbler Thanks for the review! I just reverted a commit that was part of this PR because I realized it was a hack for which the real fix is in PR #5680 .

As for Cobra, the currently used commit is sufficient for this PR. I think moving to 0.0.4 is a good idea as it brings a couple of minor fixes but should be done independently of this PR. If you want to move to 0.0.4, I can make a new PR for it. Just let me know.

So, this PR is good to commit.

Thanks!

@marckhouzam
Copy link
Member Author

Oh, and the force push was because the revert commit did not have the signed-off-by at the end so the DCO check failed. I amended the commit and forced pushed on my branch.

@bacongobbler
Copy link
Member

Awesome! Bumping Cobra would be a good idea to keep Helm 2 and Helm 3 in sync, but if it's not necessary for this PR, we can tackle bumping cobra in a followup PR. Feel free to make a new PR if you'd like :)

Thanks for responding so quickly and addressing that commit. This looks good to merge. Thanks so much for following through with this!

@bacongobbler bacongobbler added this to the 2.15.0 milestone Jun 5, 2019
@bacongobbler bacongobbler merged commit 337be4d into helm:master Jun 5, 2019
@marckhouzam marckhouzam deleted the feature/improveStatusDeleteCompletion branch June 6, 2019 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants