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

completers ∀x∈ {cmds, aliases} in powershell_completions.go #1977

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mavaddat
Copy link

@mavaddat mavaddat commented Jun 12, 2023

Where name is substituted for %[1]s, this provides PowerShell argument completer for all %[1]s commands and aliases. In case of podman, this is especially useful when user set docker as an alias.

Also, the PowerShell argument completer will now apply for %[1]s.exe if that is an entrypoint to %[1]s that is set by System.Environment. This is useful for cases (Windows contexts) where the executable has *.exe extension. (We don't want to deny argument completion when the user is employing a canonical name for the executable, which it does in the status quo.)

Does this PR introduce a user-facing change?

Yes.

Extend PowerShell argument completer to apply for each `%[1]s` command (including `*.exe`) and aliases.

See also containers/podman#18847

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2023

CLA assistant check
All committers have signed the CLA.

@marckhouzam
Copy link
Collaborator

Thanks @mavaddat.
Can you provide examples of situations that don't work before this change so I can better understand why we need this?

cc @Luap99

@mavaddat
Copy link
Author

Thanks @mavaddat. Can you provide examples of situations that don't work before this change so I can better understand why we need this?

cc @Luap99

Yes, gladly. On a Windows machine, consider an application called cliapplication with binary executable cliapplication.exe on the %PATH%. When the user types cliapp followed by Tab for tab-completion, they will get cliapplication.exe as the completion result.

ezgif com-optimize

Tab completion of the executable is provided by the env PATH as well as user-set aliases. Notice inclusion of the .exe file extension when the shell provides completion by searching the PATH.

However, the registered autocompletion provided by cobra does not include the file extension .exe nor does it include the user-set aliases. This means that if the user aliases docker for podman, for example, they will not see the completions for podman when they copy-paste (or auto complete) commands for docker. Making this especially tragic, the Register-ArgumentCompleter cmdlet specifically accepts an array of names for the completion.

In fact, I proposed this change specifically because I saw that the podman completion was deficient in this regard. I would like to be able to have completions for docker OOBE without writing another completer.

@Luap99
Copy link
Contributor

Luap99 commented Jun 13, 2023

I think this is a proper solution for aliases compared to #1621. If we can just get the list of aliases in powershell then I think it makes sense to register the completion for all aliases by default.

In my exuberance to include all the other aliases, I accidentally removed the original alias that was previously the only one being registered for autocompletion. I have restored this `'%[1]s'`.
Copy link
Author

@mavaddat mavaddat left a comment

Choose a reason for hiding this comment

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

I have tested this in PowerShell versions 3-7 continuous (3, 4, 5, 6, 7). It works as expected.

@mavaddat
Copy link
Author

How can I advance or reject this PR?

@marckhouzam
Copy link
Collaborator

I'm planning on reviewing this but you'll have to give me a couple of weeks to carve out the time.

@marckhouzam
Copy link
Collaborator

@mavaddat I'm trying to test this and it is not working for me. I have recompiled helm using this PR, but when I try to source the script for powershell completion, I get an error.
Basically, install a binary using cobra and powershell completion, don't create any aliases, and use the version of the completion script from this PR. This is what I get:

PS /Users/kmarc/git/helm> ./bin/helm completion powershell | Out-String | Invoke-Expression
Register-ArgumentCompleter:
Line |
 253 |  … CommandName ([array]$availablehelm.Name + [array]$__helmAliases.Name  …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot bind argument to parameter 'CommandName' because it is null.

Also, I just merged #1960 and now this PR needs rebasing.
@mavaddat could you wrap the CompleterBlock variable name in {} as #1960 did?

@mavaddat
Copy link
Author

mavaddat commented Jun 19, 2023

@mavaddat I'm trying to test this and it is not working for me. I have recompiled helm using this PR, but when I try to source the script for powershell completion, I get an error. Basically, install a binary using cobra and powershell completion, don't create any aliases, and use the version of the completion script from this PR. This is what I get:

PS /Users/kmarc/git/helm> ./bin/helm completion powershell | Out-String | Invoke-Expression
Register-ArgumentCompleter:
Line |
 253 |  … CommandName ([array]$availablehelm.Name + [array]$__helmAliases.Name  …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot bind argument to parameter 'CommandName' because it is null.

I was also seeing this error before I forced [array] on the two command name sources. I believe PowerShell was interpretting the first of the objects as a string, then consequently interpretting the + as string concatenation instead of an array merger, then the interpretter didn't have a way to add an array to a string, so it resolved the addition to $null which becomes a string that isn't found as a command. I have made f43b653 to mitigate this and separate out this error, if it occurs.

Also, I just merged #1960 and now this PR needs rebasing. @mavaddat could you wrap the CompleterBlock variable name in {} as #1960 did?

Done as you said. Please let me know if this is not what you meant.

Copy link
Author

@mavaddat mavaddat left a comment

Choose a reason for hiding this comment

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

@marckhouzam Were you able to check this to see if it is working for you too?

@jpmcb
Copy link
Collaborator

jpmcb commented Oct 28, 2023

This looks to be a similar solution to #2049

I'm not a windows user or powershell expert but I would like to drive consensus on one of these two solutions. There probably should have been an issue created where we could discuss different proposals.

@mavaddat - how does your solution compare to #2049?

cc @bartoncasey

@jpmcb jpmcb added area/shell-completion All shell completions triage/blocked Cannot move forward until some roadblock is lifted labels Oct 28, 2023
Incorporate @bartoncasey insight about typical alias usage in spf13#2049
@github-actions github-actions bot removed the area/shell-completion All shell completions label Oct 29, 2023
Typo in previous. Change `$_` PSItem to `$__%[2]sCmd`
@mavaddat
Copy link
Author

Thank you @bartoncasey. You make a good point that the alias may not be the command path. I have updated the PR patch-1 to incorporate your insight so that the simple alias will also work (but we check that the underlying executable is the intended target instead of assuming that there is only one name per executable).

The core logic is here:

Get-Alias | Where-Object { $_ -eq ($__CompleterTargetCmd | Resolve-Path) -or $_ -eq (Get-Command -Name $__CompleterTargetCmd).Definition}

This captures all the aliases whose underlying executable is the target for the completer we're builder.

@marckhouzam
Copy link
Collaborator

@mavaddat for some reason I cannot rebase this PR; probably because it contains a merge.
Could you squash your commits and rebase on main please?

@mavaddat
Copy link
Author

Could you squash your commits and rebase on main please?

Done. Please recheck.

@marckhouzam
Copy link
Collaborator

@mavaddat I can't get this to work as I would expect.

I recompiled kubectl using this PR.
Then I did the following in powershell (on my Mac):

Set-Alias k kubectl
k completion powershell | Out-String | Invoke-Expression

After that, I can get completion for kubectl <TAB> but not for k <TAB>

@mavaddat
Copy link
Author

@mavaddat I can't get this to work as I would expect.

I see the reason: I was using the $_ PSItem for the alias comparison, but I should've been using $_.Definition. I apologize for the mistake.

Also, could you please describe your workflow for checking this? It would be ideal that we update powershell_completions_test.go so it comprehensively checks the completer using the production-ready tests. Also, the GitHub action should invoke the testers on PR updates. I would like to implements these checks.

@marckhouzam
Copy link
Collaborator

@mavaddat I can't get this to work as I would expect.

I see the reason: I was using the $_ PSItem for the alias comparison, but I should've been using $_.Definition. I apologize for the mistake.

I'll give this a try again. Thanks for the quick fix.

Also, could you please describe your workflow for checking this?

I can only do it manually.
We don't have automated tests for actual powershell completions.

It would be ideal that we update powershell_completions_test.go so it comprehensively checks the completer using the production-ready tests. Also, the GitHub action should invoke the testers on PR updates. I would like to implements these checks.

More automation would be great. Could you clarify what you have in mind?

@marckhouzam
Copy link
Collaborator

@mavaddat It still does not work.

Let's first agree on what needs to be considered as an alias. This is what I'm understanding we are targeting:

  1. any alias pointing to kubectl or kubectl.exe
  2. any alias pointing to /<path>/kubectl or /<path>/kubectl.exe
  3. any binary named kubectl or kubectl.exe on any path

Is this correct? Are there other cases I'm missing?

From what I can see from testing on Mac, point 3 is automatically handled by Powershell (I'm running v7.3.2).
As for points 1 and 2, I think we can handle them more easily by tweaking the solution of #2049 :

# Register the completer for the command and for all aliases of the command
'%[1]s', (Get-Alias -Definition %[1]s,*/%[1]s,%[1]s.exe,*/%[1]s.exe -ErrorAction Ignore).Name | ForEach-Object {
    if ($_) {
        Register-ArgumentCompleter -CommandName $_ -ScriptBlock ${__%[2]sCompleterBlock}
    }
}

I'll followup on #2049.

@mavaddat
Copy link
Author

mavaddat commented Oct 31, 2023

what needs to be considered as an alias.

  1. any alias pointing to kubectl or kubectl.exe
  2. any alias pointing to /<path>/kubectl or /<path>/kubectl.exe
  3. any binary named kubectl or kubectl.exe on any path

Is this correct?

Yes, this is correct. A legitimate target for command completion should be any name or alias that calls the executable (i.e., the executable that relies on cobra for its command completion — let's call it The Executable).

Are there other cases I'm missing?

From what I can see from testing on Mac, point 3 is automatically handled by Powershell (I'm running v7.3.2).

Correct. The system path resolution is responsible for searching for an appropriate executable.

In PowerShell, we can call this API using the Resolve-Path cmdlet..

As for points 1 and 2, I think we can handle them more easily by tweaking the solution of #2049

The issue I foresee with #2049 is that it does not check that the system chose the correct executable. My solution was to check that the underlying executable that is called by the alias belonged to the The Executable. However, one may argue that we should not be checking that. After all, it's the system's responsibility to resolve the path. Also, attempting to check the alias's legitimacy adds code-maintenance overhead (as we are seeing in our testing back-and-forth).

@marckhouzam
Copy link
Collaborator

The issue I foresee with #2049 is that it does not check that the system chose the correct executable.

IIUC such a scenario would be of I write a script called kubectl and have it on my PATH. I think it is ok to assume that such a script skills be treated as the real kubectl binary with respect to completions.

But just in case, do you know if there’s a way for the user to unregister an argument completer if our approach causes problems?

@mavaddat
Copy link
Author

mavaddat commented Nov 2, 2023

But just in case, do you know if there’s a way for the user to unregister an argument completer if our approach causes problems?

Yes, they would re-register the argument completer for that alias/name with $null as the ScriptBlock. Please see related discussion here: https://stackoverflow.com/a/73298560/1757756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/blocked Cannot move forward until some roadblock is lifted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants