Fix windows default path overwrite #9118
Conversation
|
Hi @CharityKathure. Thanks for your PR. I'm waiting for a containerd 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. DetailsInstructions 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. |
|
Please squash the two commits to one. The change LGTM after that and the one comment! |
|
|
@thaJeztah yell at em 😆 |
|
😂 I mean, Windows moves in mysterious ways if you're used to Linux. I should look at some canonical docs for that, and link it in the code (for future reference). I think we also may want to update the OCI image / runtime specs to mention this (ISTR it mentions the path to set for Linux) Any useful info welcome on that matter (feel free to comment on my PR) |
3f04db3 to
321bf97
Compare
|
cc @kevpar @kiashok. AFAIR the OS did set/use a default path so this seems correct. We don't seem to set a default path in some spots that we would for linux either after perusing. https://github.com/microsoft/hcsshim/blob/27df1b95b69faaeca97de86d25d68f48f89bc0b9/internal/cmd/cmd.go#L118-L125 |
|
I referred to John Howard's commit in moby/moby@3c177dc (which called this out moby/moby@3c177dc#diff-4f8b57539bb09b7370a8ebaf4557bdcf65a63f2443edf4e0d42eecfeeacef900R13-R15) |
|
@CharityKathure could you add a test for this? |
We could move this function to a new oci/spec_opts_nonwindows.go file if we do not intend it to be used on windows. |
oci/spec_opts.go
Outdated
|
|
||
| // WithDefaultPathEnv sets the $PATH environment variable to the | ||
| // default PATH defined in this package. | ||
| func WithDefaultPathEnv(_ context.Context, _ Client, _ *containers.Container, s *Spec) error { |
There was a problem hiding this comment.
If this function is not intended to be used on windows, we should probably move this out to a new spec_opts_nonwindows.go file and override this function to return nil/unsupported in spec_opts_windows.go
98e56fe to
5ec0a26
Compare
Windows Containers have a default path already configured at bootup. WithDefaultPathEnv overwrites this with a unix path Signed-off-by: charitykathure <kathurecharity505@gmail.com>
5ec0a26 to
13dcf55
Compare
Windows Containers have a default path already configured at bootup. WithDefaultPathEnv overwrites this with a unix path Signed-off-by: charitykathure <kathurecharity505@gmail.com>
…om/CharityKathure/containerd into windows-default-path-overwrite-fix
…h upstream containerd/main update fork-external/main branch to upstream containerd/main at commit f90f80d Related work items: containerd#8736, containerd#8861, containerd#8924, containerd#8934, containerd#9027, containerd#9076, containerd#9104, containerd#9118, containerd#9141, containerd#9155, containerd#9177, containerd#9183, containerd#9184, containerd#9186, containerd#9187, containerd#9191, containerd#9200, containerd#9205, containerd#9211, containerd#9214, containerd#9215, containerd#9221, containerd#9223, containerd#9228, containerd#9231, containerd#9234, containerd#9242, containerd#9246, containerd#9247, containerd#9251, containerd#9253, containerd#9254, containerd#9255, containerd#9268
Fixes: #containerd/nerdctl#2485
Windows Containers have a default path already configured at bootup. WithDefaultPathEnv overwrites this with a unix path