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

Jump to bottom

Allow user to add completion for powershell alias #1621

Merged
merged 1 commit into from Oct 3, 2022
Merged

Allow user to add completion for powershell alias #1621

merged 1 commit into from Oct 3, 2022

Conversation

marckhouzam
Copy link
Collaborator

I ran across a possible problem with using powershell completion with aliases. I can't recall where it was though, but the situation is pretty simple.

IIUC, when a user has an alias in powershell, she needs to register that alias for completion using:

PS> Register-ArgumentCompleter -CommandName 'aliasname' -ScriptBlock <completionBlock>

The problem is that the <completionBlock> is currently not accessible in our powershell script.

To address this, this commit stores the completion logic into a scriptblock variable which can easily be accessed by the
user to register aliases.

For example, if the user defines an alias for helm she will be able to register an alias like this:

PS> sal h helm
PS> Register-ArgumentCompleter -CommandName 'h' -ScriptBlock $__helmCompleterBlock

@Luap99 does this make sense to you? I may be misunderstanding something fundamental about aliases and powershell...

@Luap99
Copy link
Contributor

Luap99 commented Mar 17, 2022

How does this work with the other shells at the moment?

I do not use powershell any more but I think this will work. Back when I worked on this there weren't many projects with powershell completion so I am not sure if there is maybe some pseudo standard which can/should be followed.

@marckhouzam
Copy link
Collaborator Author

This whole idea came out of a comment from the PR that added Powershell completion to kubectl: kubernetes/kubernetes#103758 (comment)

There was a project that implemented a hard-coded script for powershell completion for kubectl which handled aliases in this way. This is the specific line that inspired me: https://github.com/mziyabo/PSKubectlCompletion/blob/f6f28153a616e14789a3a7fbcb493ff7f1f1ae85/PSKubectlCompletion.psm1#L20

@Luap99
Copy link
Contributor

Luap99 commented Mar 18, 2022

I personally do not like to pollute the global variable scope in the shell but the variable name seems reasonable unique so it should be fine. Since there is a legitimate use case for this and I cannot offer a better solution I am fine with this PR.

@marckhouzam marckhouzam added kind/feature A feature request for cobra; new or enhanced behavior area/shell-completion All shell completions lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready labels Mar 26, 2022
@jpmcb
Copy link
Collaborator

jpmcb commented Mar 29, 2022

Hmmmmm anyone out in the community have an easy way to test this on Windows? I'm without my windows box at the moment.

@jpmcb jpmcb added the help-wanted An issue that the maintainers would like help resolving label Mar 29, 2022
@github-actions
Copy link

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
    You can:
  • Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening.

@marckhouzam
Copy link
Collaborator Author

Still valid

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added size/M Denotes a PR that chanes 24-99 lines and removed area/shell-completion All shell completions labels Sep 18, 2022
@marckhouzam
Copy link
Collaborator Author

I fixed this PR which is still relevant.

Luap99
Luap99 reviewed Sep 19, 2022
View reviewed changes
powershell_completions.go Outdated Show resolved Hide resolved
@marckhouzam
Allow user to add completion for powershell alias
6ba7ebb 
When a user has an alias in powershell, she will need to register that
alias for completion.  To make that possible, we store the completion
logic into a scriptblock variable which can easily be accessed by the
user to register aliases.

For example, if the user defines an alias for `helm`:
   PS> sal h helm
she will need to register the alias like so:
   PS> Register-ArgumentCompleter -CommandName 'h' -ScriptBlock $__helmCompleterBlock

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
Luap99
Luap99 reviewed Sep 19, 2022
View reviewed changes
Copy link
Contributor

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

jpmcb
jpmcb approved these changes Oct 3, 2022
View reviewed changes
Copy link
Collaborator

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

@marckhouzam @Luap99 - I see this has the lgtm label: I am still without a windows box suitable for testing :( is this good to go? Code looks good to me and If you feel ok with this Marc, go ahead and self merge

@marckhouzam
Copy link
Collaborator Author

Yeah, it is ready. I am able to test it on powershell on Mac, and @Luap99 is happy with the change. Will merge.

@marckhouzam marckhouzam added this to the 1.6.0 milestone Oct 3, 2022
@marckhouzam marckhouzam merged commit d4040ad into spf13:main Oct 3, 2022
@marckhouzam marckhouzam deleted the feat/pwshAliases branch October 3, 2022 17:06
@umarcor umarcor mentioned this pull request Oct 3, 2022
Closed
@cloud-66
Copy link

cloud-66 commented Feb 17, 2023 •
edited

Sorry. But why doesn't it work for me?

# PowerShell auto-completion
kubectl completion powershell | Out-String | Invoke-Expression
Set-Alias -Name k -Value kubectl
Register-ArgumentCompleter -CommandName k -ScriptBlock $__kubectlCompleterBlock

@marckhouzam
Copy link
Collaborator Author

What version of kubectl are you using? I believe you need 1.26 or higher.

@cloud-66
Copy link

What version of kubectl are you using? I believe you need 1.26 or higher.

Yeah. Sorry to bother you. Kubectl 1.26+ depends on cobra 1.6.0 with this pr. thank you it works .

hoshsadiq pushed a commit to zulucmd/zulu that referenced this pull request May 16, 2023
@marckhouzam @hoshsadiq
Allow user to add completion for powershell alias
34d3bfd 
When a user has an alias in powershell, she will need to register that
alias for completion.  To make that possible, we store the completion
logic into a scriptblock variable which can easily be accessed by the
user to register aliases.

For example, if the user defines an alias for `helm`:
   PS> sal h helm
she will need to register the alias like so:
   PS> Register-ArgumentCompleter -CommandName 'h' -ScriptBlock $__helmCompleterBlock

Merge spf13/cobra#1621
@Luap99 Luap99 mentioned this pull request Jun 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 left review comments

@jpmcb jpmcb jpmcb approved these changes

Assignees
No one assigned
Labels
help-wanted An issue that the maintainers would like help resolving kind/feature A feature request for cobra; new or enhanced behavior lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready size/M Denotes a PR that chanes 24-99 lines
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@marckhouzam @Luap99 @jpmcb @CLAassistant @cloud-66