Support exec plugin on Windows #3027
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Welcome @gochist! |
|
Hi @gochist. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XS
|
/assign @phanimarupaka |
|
/assign @monopole |
|
/unassign @phanimarupaka |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| if f.Mode()&0111 == 0000 { | ||
| // In Windows, it is not possible to determine whether a | ||
| // file is executable through file mode. | ||
| if f.Mode()&0111 == 0000 && runtime.GOOS != "windows" { |
There was a problem hiding this comment.
Can you add a test for this behavior?
There was a problem hiding this comment.
@Shell32-Natsu Thank you for the review. I'm not good at Go development, but I tried to add a test. I tried to skip the test in Windows environment, what do you think?
There was a problem hiding this comment.
@gochist To line 55, can you add this:
// TODO: provide for setting the executable FileMode bit on Windows
// The (fs *fileStat) Mode() (m FileMode) {} function in
// in https://golang.org/src/os/types_windows.go
// lacks the ability to set the FileMode executable bit in response
// to file data on Windows.
I did a quick search for an issue upstream in the golang repo but couldn't find one, maybe you can try?
https://github.com/golang/go/issues?q=is%3Aissue+is%3Aopen+Windows++filemode
seems like we'd need Go functions to read
// using perhaps https://docs.microsoft.com/en-us/windows/win32/secauthz/access-control-entries?redirectedfrom=MSDN
and we're not going to attempt that in kustomize.
There was a problem hiding this comment.
then i think we should pull it in, and unblock windows use.
There was a problem hiding this comment.
@monopole Thank you. I've addressed your review. And I will open an issue in the golang repo within a few days.
There was a problem hiding this comment.
@monopole I've opened an issue, golang/go#41809 in the golang repo.
k8s-ci-robot
added
size/M
gochist
force-pushed
the
windows-exec
branch
from
d13160b
to
85f79ed
Compare
| @@ -89,3 +92,32 @@ metadata: | |||
| t.Fatalf("unexpected arg array: %#v", p.Args()) | |||
| } | |||
| } | |||
|
|
|||
| func TestExecPlugin_ErrIfNotExecutable(t *testing.T) { | |||
There was a problem hiding this comment.
@monopole Could you take a look. I think this test should work but not sure about the practice that skip it on Windows.
There was a problem hiding this comment.
yes, this is ok in this case.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gochist The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
lgtm
As discussed on #2924, we are now aware that the exec plugin is not available on Windows. This is because we can't check if the plugin is executable via file mode on Windows.
So, I changed the conditions for loading exec plugin so that the plugin can be used in Windows.