Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Update cobra to support plugin flag completion #1251

Merged
merged 1 commit into from Jan 19, 2022

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Nov 29, 2021

What this PR does / why we need it

Updates cobra to a version that includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Without this fix, the completion handler inside cobra is interpreted as returning flags for the command hosting the plugin. With this fix, when flag parsing is disabled for a commend, the custom completion hook is invoked, which the Tanzu CLI uses to collect completion info from plugins.

Describe testing done for PR

I used the special hook provided by cobra that is used by shell completions to compare the suggestions with and without this PR.

tanzu __complete package install -

Without this PR (plugin flag completions are not available):

:4
Completion ended with directive: ShellCompDirectiveNoFileComp

With this PR (plugin flag completions are available):

--package-name  Name of the package to be installed
-p      Name of the package to be installed
--version       Version of the package to be installed
-v      Version of the package to be installed
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

Release note

Plugins using native shell completion will now offer suggestions for flag names

PR Checklist

  • Squash the commits into one or a small number of logical commits
  • Use good commit messages
  • Ensure PR contains terms all contributors can understand and links all contributors can access

Additional information

Special notes for your reviewer

There is no tagged version of cobra that contains this patch, this commit is the HEAD of the master branch. We can switch back to tagged releases once there is a new release.

cc @jpmcb (who helped land the upstream PR)

@scothis scothis requested a review from a team as a code owner November 29, 2021 18:24
Copy link
Contributor

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

I'm currently working on cutting a cobra release for v1.2.2

I propose we wait for a release instead of pinning to an arbitrary sha.

@scothis
Copy link
Contributor Author

scothis commented Nov 29, 2021

@jpmcb I can update after cobra releases or if someone else (Dependabot) beats me to it we can close this PR. Either way, I verified that the Tanzu CLI completion issue is fixed by updating cobra.

@jpmcb
Copy link
Contributor

jpmcb commented Nov 29, 2021

I verified that the Tanzu CLI completion issue is fixed by updating cobra.

Awesome!! Very glad this fix worked for you :D

I'll see how pushing through a release goes (hopefully by this week). If we need this feature to be in a tanzu-framework release, then we can have this go in intermittently before I get a release out

@jpmcb
Copy link
Contributor

jpmcb commented Dec 14, 2021

v1.3.0 has been cut! https://github.com/spf13/cobra/releases/tag/v1.3.0

Let me know if y'all need any support on this one!!

@scothis
Copy link
Contributor Author

scothis commented Dec 14, 2021

Rebased on main and updated cobra to 1.3.0

@scothis
Copy link
Contributor Author

scothis commented Jan 11, 2022

Rebased again

@scothis scothis requested a review from a team as a code owner January 11, 2022 22:55
This version includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@miclettej
Copy link
Contributor

@giri-varma PTAL, thanks!

@giri-varma
Copy link
Contributor

I'm currently working on cutting a cobra release for v1.2.2

I propose we wait for a release instead of pinning to an arbitrary sha.

@jpmcb I see that the change has been incorporated. Do you see any issues merging this?

Copy link
Contributor

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

From the cobra perspective, yes this is fine!

@giri-varma giri-varma added the ok-to-merge PRs should be labelled with this before merging label Jan 19, 2022
@giri-varma giri-varma self-requested a review January 19, 2022 17:43
@vuil vuil merged commit 8acce0b into vmware-tanzu:main Jan 19, 2022
@scothis scothis deleted the bump-cobra branch January 19, 2022 23:18
@marckhouzam marckhouzam mentioned this pull request Jun 21, 2022
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging orange orange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants