Remove `.exe` suffix if any in completion #24966

silver886 · 2025-01-08T06:30:47Z

In shell completion, there is .exe suffix on Windows and this break several shells.

Relates to #16499.
It seems didn't fix at the time but waiting for a fix in cobra.

Fixed a bug that prevents shell completion on Windows under some circumstances.

openshift-ci · 2025-01-08T06:30:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: silver886
Once this PR has been reviewed and has the lgtm label, please assign giuseppe for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Luap99

Thanks for the fix, please add the description from the PR also to the commit message.

It would also be a good idea to have some form of test, I think podman --help might be enough to run on windows and check the Usage string?
@l0rd Any idea where this could be added?

Luap99 · 2025-01-08T10:15:57Z

cmd/podman/root.go

@@ -62,7 +62,7 @@ Options:

 var (
 	rootCmd = &cobra.Command{
-		Use:                   filepath.Base(os.Args[0]) + " [options]",
+		Use:                   strings.TrimSuffix(filepath.Base(os.Args[0]), ".exe") + " [options]",


Can you add a comment here why .exe is trimmed and add a link to the issue.
If the line is moved or edited again it may no longer be easy to find in the git history.

Comment and links to issue and pr are added.
I also added description to the commit message.

baude · 2025-01-08T21:49:45Z

LGTM other than the comment

Luap99

Thanks please also squash the commits as this is one change.

l0rd · 2025-01-09T17:43:11Z

It would also be a good idea to have some form of test, I think podman --help might be enough to run on windows and check the Usage string? @l0rd Any idea where this could be added?

@Luap99 do you mean something similar to test/system/600-completion.bats but simpler and that runs on windows? I would probably try to add it in test/e2e.

I have manually tested this PR, and completion works fine on Powershell for podman, with and without .exe suffix.

One minor comment:

It was waiting for a fix in cobra upstream, but since that has not happened yet...

I may be missing something but it looks like the cobra issue has been fixed and that's a Podman specific issue now (kubectl completion for instance works fine with and without .exe). Maybe it would be helpful to document why we are not using the hardcoded podman and we need os.Args[0] instead?

Luap99 · 2025-01-10T10:45:27Z

It would also be a good idea to have some form of test, I think podman --help might be enough to run on windows and check the Usage string? @l0rd Any idea where this could be added?

@Luap99 do you mean something similar to test/system/600-completion.bats but simpler and that runs on windows? I would probably try to add it in test/e2e.

No that test is useless for a bug like this it test actuall completion function that are the same on all platforms so there is no need to duplicate that.
As I said above you should be able to extract that base command name from the podman --help usage string which should be more than enough to ensure the command name is sane.

Maybe it would be helpful to document why we are not using the hardcoded podman and we need os.Args[0] instead?

Because we use podman-remote as well for the same cli.

l0rd · 2025-01-10T11:06:57Z

@Luap99 oh ok that's clear now, thanks. Then a new test pkg/machine/e2e/help_test.go should work (@silver886 you can get inspiration from pkg/machine/e2e/info_test.go).

silver886 · 2025-01-10T20:45:36Z

@l0rd am I looking at the right document for unit testing?
https://github.com/containers/podman/blob/main/test/README.md#unittest-for-testutils

It seems like make localunit doesn't run unit tests for pkg/machine/e2e as it

Will skip:
  ./hack/podman-registry-go
  ./pkg/bindings/images
  ./pkg/bindings/test
  ./pkg/machine/e2e
  ./test/e2e

And in the pipeline, that's the only command for unit test.
https://cirrus-ci.com/task/5707589457018880?logs=main#L32

l0rd · 2025-01-10T21:21:43Z

@silver886 no, that's not the correct documentation. That's hard to find, but the proper documentation for running podman remote e2e tests is here: pkg/machine/e2e/README.md.

silver886 · 2025-01-11T03:28:06Z

@l0rd @Luap99 I added the unit test and it passed on Windows.

l0rd

@silver886 thank you very much for the update and the tests. It looks great 👏. We are almost there. I have provided some suggestions, please have a look.

l0rd · 2025-01-11T11:30:33Z

pkg/machine/e2e/help_test.go

+	. "github.com/onsi/gomega/gexec"
+)
+
+var _ = Describe("podman completion powershell", func() {


The command name is usually used in the description. If we add more podman help tests, they will be added here.

Suggested change

var _ = Describe("podman completion powershell", func() {

var _ = Describe("podman help", func() {

l0rd · 2025-01-11T11:45:02Z

pkg/machine/e2e/help_test.go

+
+var _ = Describe("podman completion powershell", func() {
+
+	It("completion powershell", func() {


This test will be run on Linux and macOS too. We may specify that this is a Windows only tests so that it doesn't run there but I won't do that as I think there is value in testing it on all OSes.
Instead, I would change the description and adapt the test to ensure that the command matches the string podman or podman-remote. I would also add a comment explaining why we are doing this test on the basic command (because command completion doesn't work otherwise) anda link to the issue you are fixing.

Suggested change

It("completion powershell", func() {

It("podman usage base command is podman or podman-remote, without extension", func() {

l0rd · 2025-01-13T12:32:05Z

pkg/machine/e2e/config_help_test.go

+}
+
+func (i *helpMachine) buildCmd(m *machineTestBuilder) []string {
+	cmd := []string{"machine", "info", "--help"}


Suggested change

cmd := []string{"machine", "info", "--help"}

cmd := []string{"help"}

l0rd · 2025-01-13T12:34:48Z

cmd/podman/root.go

+		//
+		// It was waiting for a fix in cobra upstream, but since that has not happened yet,
+		// a fix was implemented in [#24966](https://github.com/containers/podman/pull/24966).


I don't think that this is necessary and anyway the cobra upstream issue has been fixed.

Suggested change

//

// It was waiting for a fix in cobra upstream, but since that has not happened yet,

// a fix was implemented in [#24966](https://github.com/containers/podman/pull/24966).

silver886 · 2025-01-14T00:48:21Z

@l0rd thank you. I have a terrible sense of naming.

l0rd · 2025-01-14T11:21:57Z

@l0rd thank you. I have a terrible sense of naming.

Eh eh, naming is hard :-)

Fantastic. Can you please squash the 3 commits into a single one and force push? And please make sure to sign your commit so that the DCO check is passing.

Signed-off-by: Leo Liu <[email protected]> Add comment In shell completion, there is `.exe` suffix on Windows and this does not provide same experience across platforms, containers#16499 Signed-off-by: Leo Liu <[email protected]> Create unit test for `.exe` suffix removal Signed-off-by: Leo Liu <[email protected]> Update comments Signed-off-by: Leo Liu <[email protected]>

openshift-ci bot added the release-note label Jan 8, 2025

Luap99 reviewed Jan 8, 2025

View reviewed changes

Luap99 reviewed Jan 9, 2025

View reviewed changes

silver886 force-pushed the patch-2 branch from df776ca to 6876e5d Compare January 10, 2025 03:13

github-actions bot added the machine label Jan 11, 2025

silver886 force-pushed the patch-2 branch 2 times, most recently from 604cf2e to 78b109e Compare January 11, 2025 03:04

l0rd reviewed Jan 13, 2025

View reviewed changes

silver886 force-pushed the patch-2 branch from ec761d0 to a91aa36 Compare January 14, 2025 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `.exe` suffix if any in completion #24966

Remove `.exe` suffix if any in completion #24966

silver886 commented Jan 8, 2025

openshift-ci bot commented Jan 8, 2025

Luap99 left a comment

Luap99 Jan 8, 2025

silver886 Jan 8, 2025

baude commented Jan 8, 2025

Luap99 left a comment

l0rd commented Jan 9, 2025 •

edited

Loading

Luap99 commented Jan 10, 2025

l0rd commented Jan 10, 2025

silver886 commented Jan 10, 2025 •

edited

Loading

l0rd commented Jan 10, 2025

silver886 commented Jan 11, 2025

l0rd left a comment

l0rd Jan 11, 2025

l0rd Jan 11, 2025

l0rd Jan 13, 2025

l0rd Jan 13, 2025

silver886 commented Jan 14, 2025

l0rd commented Jan 14, 2025

	var _ = Describe("podman completion powershell", func() {
	var _ = Describe("podman help", func() {


		var _ = Describe("podman completion powershell", func() {

		It("completion powershell", func() {

	It("completion powershell", func() {
	It("podman usage base command is podman or podman-remote, without extension", func() {

	cmd := []string{"machine", "info", "--help"}
	cmd := []string{"help"}

	//
	// It was waiting for a fix in cobra upstream, but since that has not happened yet,
	// a fix was implemented in [#24966](https://github.com/containers/podman/pull/24966).

Remove .exe suffix if any in completion #24966

Are you sure you want to change the base?

Remove .exe suffix if any in completion #24966

Conversation

silver886 commented Jan 8, 2025

openshift-ci bot commented Jan 8, 2025

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Jan 8, 2025

Choose a reason for hiding this comment

silver886 Jan 8, 2025

Choose a reason for hiding this comment

baude commented Jan 8, 2025

Luap99 left a comment

Choose a reason for hiding this comment

l0rd commented Jan 9, 2025 • edited Loading

Luap99 commented Jan 10, 2025

l0rd commented Jan 10, 2025

silver886 commented Jan 10, 2025 • edited Loading

l0rd commented Jan 10, 2025

silver886 commented Jan 11, 2025

l0rd left a comment

Choose a reason for hiding this comment

l0rd Jan 11, 2025

Choose a reason for hiding this comment

l0rd Jan 11, 2025

Choose a reason for hiding this comment

l0rd Jan 13, 2025

Choose a reason for hiding this comment

l0rd Jan 13, 2025

Choose a reason for hiding this comment

silver886 commented Jan 14, 2025

l0rd commented Jan 14, 2025

Remove `.exe` suffix if any in completion #24966

Remove `.exe` suffix if any in completion #24966

l0rd commented Jan 9, 2025 •

edited

Loading

silver886 commented Jan 10, 2025 •

edited

Loading