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

HIP for descriptions for custom completions #161

Merged
merged 2 commits into from Feb 18, 2021

Conversation

marckhouzam
Copy link
Member

@marckhouzam marckhouzam commented Nov 15, 2020

This HIP is a proposal to add descriptions to helm's custom completions.
For quick access: https://github.com/VilledeMontreal/community/blob/hip/compCustomDescriptions/hips/hip-0008.md

For example:

$ helm history n<TAB>
nginx   -- nginx-6.0.2 -> deployed
nginx2  -- nginx-ingress-1.41.2 -> deployed

instead of simply:

$ helm history n<TAB>
nginx  nginx2

I wasn't sure a HIP was really necessary, however having one (assuming it is accepted), will make it easier to get input on the information that should be added to each type of completion (see the Specifications section).

Note that such support would apply to zsh and fish shells, as bash does not (currently) support descriptions for completions.

Proposal to add descriptions to helm's custom completions.
This would apply to zsh and fish shells but not to bash, as bash does
not (currently) support descriptions for completions.

For example:

$ helm history n<TAB>
nginx   -- nginx-6.0.2 -> deployed
nginx2  -- nginx-ingress-1.41.2 -> deployed

instead of simply:

$ helm history n<TAB>
nginx  nginx2

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
@bacongobbler
Copy link
Member

Providing background context on why Helm completion should add descriptions to all commands moving forward is a good use of a HIP IMO. It's great that you submitted one!

Thank you. I have a few meetings to attend right now but will take a closer look this afternoon.

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.

This looks great! I left some feedback on how we could address the chart description field.

Approved as HIP 8. Please change the HIP number and filename to match. Thanks!

hips/hip-9999.md Outdated Show resolved Hide resolved
hips/hip-9999.md Outdated

Is there other *useful* information to show? If not, it may be more useful
not to add a description in this case to allow to fit more chart completion
choices on the screen.
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 I can agree with this point. The description field would be a great first choice. However, I believe there is no "hard" limit on the number of accepted characters in a description field.

What if the description was short-wrapped with ellipsis similar to what we do with helm search repo?

><> helm search repo wordpress
NAME                    CHART VERSION   APP VERSION     DESCRIPTION
stable/wordpress        9.0.3           5.3.2           DEPRECATED Web publishing platform for building...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the shell does the ellipsis automatically, so I'll give it a try in the reference implementation and we can discuss how it feels during that PR review. I'll update the HIP to propose using the description for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify, fish shell does an ellipsis:

bitnami/cassandra  (Apache Cassandra is a free and open-source distributed database manage…)
bitnami/common  (A Library Helm Chart for grouping common logic between bitnami charts. Th…)
bitnami/consul  (Highly available and distributed service discovery and key-value store de…)
bitnami/contour                                  (Contour Ingress controller for Kubernetes)

while zsh simply truncates:

bitnami/cassandra  -- Apache Cassandra is a free and open-source distributed database ma
bitnami/common     -- A Library Helm Chart for grouping common logic between bitnami cha
bitnami/consul     -- Highly available and distributed service discovery and key-value s
bitnami/contour    -- Contour Ingress controller for Kubernetes

For some chart descriptions, the amount of space available on a single line is not sufficient to convey complete information, as can be seen in the example above, especially for consul.

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

I've updated this PR based on @bacongobbler input.
There's also a PR starting the reference implementation (helm/helm#9243)
Since this PR is approved, should it be merged? (I'm not a maintainer on this project).

@bacongobbler
Copy link
Member

bacongobbler commented Feb 3, 2021

Just for clarification, HIPs require two approvals before they can be merged. From https://github.com/helm/community/blob/master/hips/hip-0001.md#proposal-review--resolution:

Feature and Informational proposals require at least 2 approvals from project maintainers. Process proposals require at least 2 approvals from Helm org maintainers.

This is different than the usual Helm PR approval process where only one approval is required if the PR comes from a maintainer. The justification here being that HIPs requires thoughtful consideration and should form a consensus among a small minority of the maintainers. The actual code itself requires only one approval as a mentoring/QA process.

Would you mind asking another Helm maintainer to take a look at this HIP?

@marckhouzam
Copy link
Member Author

Feature and Informational proposals require at least 2 approvals from project maintainers. Process proposals require at least 2 approvals from Helm org maintainers.

Thanks and sorry about that, I had missed that point.

@mattfarina would you mind having a look? We had verbally discussed the idea of adding descriptions to helm custom completion (chart info, repo info, etc) a little while ago.
(I can't formally ask for a review on GitHub on this project)

Copy link
Contributor

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

lgtm

@mattfarina
Copy link
Contributor

@marckhouzam you've got your 2 approvals. I'm going to merge and any changes can be made in a follow-up PR.

@mattfarina mattfarina merged commit 17ccf69 into helm:master Feb 18, 2021
@marckhouzam marckhouzam deleted the hip/compCustomDescriptions branch February 18, 2021 23:41
@marckhouzam
Copy link
Member Author

Thanks @mattfarina and @bacongobbler for taking the time to look at this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants