Run plugin on Windows

[MIBc]

Fixes #737

• Use exec.Command instend of syscall.Exec for windows.
• Fix a bug in plugin handler test when running it on windows.
• Fix typo.

Release Note:

• 🐛 Bug fix
  Using exec.Command instend of syscall.Exec for windows.

[knative-prow-robot]

Hi @MIBc. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mattmoor]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

[navidshaikh]

/ok-to-test

[rhuss]

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

@mattmoor Could we please switch off this check ? A mentioned elsewhere we are using goimports as part of our regular build. Goimports has a slightly different opinion of how imports are organized (no idea why), but having this suggestions are too noisy imo causing unnecessary friction. Also, the next build will rearrange it again.

Just out of curiosity: Why did you stop using a dedicated bot account and use your real account for both activities (manual and automated PR/reviews) ? Asking, because quite some stats (like in TOC meetings or also when it comes for the TOC election process) are based on contributions, not sure whether automated commits (like these) are diluting these stats.

[rhuss]

@MIBc please don't apply the reformatting suggestions, you will see if you rerun build.sh the imports will get rearranged again.

[maximilien]

Thanks for this contribution. Since I don't have a Windows system to try this I have to assume your changes are correct and needed :)

Otherwise LGTM. See my one comment left. Hopefully we can merge this soon. Thanks again.

[dsimansk]

@MIBc @maximilien
I have access to Windows on my home PC. So I've tried to check the current PR, but I'm still seeing a weird error message. Even though that plugin is executed, there's also and unknown command displayed afterwards.

Have you seen such behavior during your tests? Otherwise the PR looks good.

C:\Users\StyX\Desktop>kn-windows-amd64.exe hello.bat --plugins-dir=C:\Users\StyX\AppData\Roaming\kn\plugins
"Hello!"
unknown command "hello.bat" for "kn"

kn-hello.bat

@ECHO OFF
ECHO "Hello!"  

[MIBc]

@MIBc @maximilien
I have access to Windows on my home PC. So I've tried to check the current PR, but I'm still seeing a weird error message. Even though that plugin is executed, there's also and unknown command displayed afterwards.

Have you seen such behavior during your tests? Otherwise the PR looks good.

C:\Users\StyX\Desktop>kn-windows-amd64.exe hello.bat --plugins-dir=C:\Users\StyX\AppData\Roaming\kn\plugins
"Hello!"
unknown command "hello.bat" for "kn"

kn-hello.bat

@ECHO OFF
ECHO "Hello!"  

Plugin's name should begin with kn. Like kn-hello.bat

[dsimansk]

@MIBc I had to add the following lines that're also part of kubectl implementation.

Per conversation with @navidshaikh. He found out that this behaviour is reproducible on Linux as well, if exec.Command is used instead of syscall.Exec.

➜  client git:(pr-738) ✗ git --no-pager diff
diff --git a/pkg/kn/commands/plugin/handler.go b/pkg/kn/commands/plugin/handler.go
index 1d05169..7ee7c4e 100644
--- a/pkg/kn/commands/plugin/handler.go
+++ b/pkg/kn/commands/plugin/handler.go
@@ -100,7 +100,11 @@ func (h *DefaultPluginHandler) Execute(executablePath string, cmdArgs, environme
 		cmd.Stderr = os.Stderr
 		cmd.Stdin = os.Stdin
 		cmd.Env = environment
-		return cmd.Run()
+		err := cmd.Run()
+		if err == nil {
+			os.Exit(0)
+		}
+		return err
 	}
 	return syscall.Exec(executablePath, cmdArgs, environment)
 }

[MIBc]

Yes. It needs an exit code when using exec.Command on windows .

[MIBc]

/retest

[dsimansk]

Thanks!
/lgtm

[maximilien]

/lgtm

[rhuss]

When this PR is merged, can we cherry pick the commit also to release-0.13 branch so that it gets part of 0.13.1 ?

[navidshaikh]

+1 for getting this for 0.13.1

@MIBc : could you quickly fix the typo in CHANGELOG ?
or we can merge the PR and fix in cherry pick PR too.

[rhuss]

/lgtm

[navidshaikh]

/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MIBc, navidshaikh

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [navidshaikh]

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