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

Add powershell completions #9307

Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Feb 10, 2021

Add support for generating powershell completion files. This is especially
useful for people using the podman remote client on windows.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2021
@jwhonce
Copy link
Member

jwhonce commented Feb 10, 2021

@Luap99 LGTM, you have some formatting issues.

Copy link
Collaborator

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

I can't really review this: I've never actually used MS Windows. I have no way to know if this will work, or to verify your Get-Help or other man page instructions.

I found one super-minor typo, and have one question for the team at large.

I did confirm your Makefile changes, and did consistency checks on man pages. All looks good.

One question: the .ps1 files you're submitting seem to have been generated with this version of cobra:

$ ./bin/podman completion powershell|diff -u - completions/powershell/podman.ps1
[no output, exit status 0]

The other completion files are generated using cobra with various unmerged patches (sorry, I can't keep track of the precise details). My understanding is that the .ps1 files aren't used at all, but if someone Windows-savvy figures out how to distribute them, would it be useful for you to submit .ps1 files including your custom cobra changes?

docs/source/markdown/podman-completion.1.md Outdated Show resolved Hide resolved
@@ -59,7 +59,7 @@ Options:

var (
rootCmd = &cobra.Command{
Use: path.Base(os.Args[0]) + " [options]",
Use: filepath.Base(os.Args[0]) + " [options]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This tentatively LGTM, and seems to be consistent with other uses of Base in the code, but it's the sort of change I'd like a lot of eyeballs on. @containers/podman-maintainers PTAL.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is important for windows since the path package splits only on a regular slash even on windows.

@Luap99
Copy link
Member Author

Luap99 commented Feb 10, 2021

The other completion files are generated using cobra with various unmerged patches (sorry, I can't keep track of the precise details). My understanding is that the .ps1 files aren't used at all, but if someone Windows-savvy figures out how to distribute them, would it be useful for you to submit .ps1 files including your custom cobra changes?

I implemented the powershell completion upstream spf13/cobra#1208 All my changes are in there :)

@Luap99
Copy link
Member Author

Luap99 commented Feb 10, 2021

I can't really review this: I've never actually used MS Windows. I have no way to know if this will work, or to verify your Get-Help or other man page instructions.

You could theoretically install powershell on linux. Not that you would ever want or need that.
https://docs.microsoft.com/en-gb/powershell/scripting/install/installing-powershell-core-on-linux?view=powershell-7.1

@TomSweeneyRedHat
Copy link
Member

I've a few man page changes to consider. The code looks fine with an eyeball revied, but like @edsantiago, I don't have an easy way to test it at the moment.

@vrothberg
Copy link
Member

Friendly ping (going through PRs at the moment)

@Luap99
Copy link
Member Author

Luap99 commented Feb 25, 2021

Updated with @TomSweeneyRedHat doc fixes.

If you want to test it you can run the following. Make sure you have the podman service is running.

$ make podman-remote-static
$ podman run -it -v $(pwd)/bin/podman-remote-static:/bin/podman-remote -v /run/user/1000/podman/podman.sock:/run/podman/podman.sock   --security-opt label=disable  --rm mcr.microsoft.com/powershell
PowerShell 7.1.2
Copyright (c) Microsoft Corporation.

https://aka.ms/powershell
Type 'help' to get help.

PS /> podman-remote completion powershell | Out-String | Invoke-Expression
PS /> podman-remote [TAB]

@rhatdan
Copy link
Member

rhatdan commented Feb 26, 2021

LGTM

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@edsantiago
Copy link
Collaborator

/lgtm
/hold

Sorry about spacing out on this. Holding because I get nervous when merging from an old base; I'll leave you the choice of unholding or rebasing. If you rebase, I promise to re-lgtm ASAP.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2021
Add support for generating powershell completion files. This is especially
useful for people using the podman remote client on windows.

[NO TESTS NEEDED]

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2021
@Luap99
Copy link
Member Author

Luap99 commented Mar 29, 2021

I lost track of this as well. I rebased, better than breaking the CI.

@edsantiago
Copy link
Collaborator

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2021
@edsantiago
Copy link
Collaborator

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit 08eab3f into containers:master Mar 29, 2021
@Luap99 Luap99 deleted the powershell-completion branch March 29, 2021 15:17
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants