-
Notifications
You must be signed in to change notification settings - Fork 597
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
Fix and enable CI unit testing for windows #3566
Conversation
@djdongjin if you are around - this was previously dependent / blocked by other under-review PRs (eg: part5), but I rebased that differently, and could be merged now. |
- if: ${{ matrix.goos=='windows' }} | ||
uses: actions/[email protected] | ||
with: | ||
repository: containerd/containerd | ||
ref: v1.7.23 | ||
path: containerd | ||
fetch-depth: 1 | ||
- if: ${{ matrix.goos=='windows' }} | ||
name: "Set up CNI" | ||
working-directory: containerd | ||
run: GOPATH=$(go env GOPATH) script/setup/install-cni-windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only windows requires this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic trick. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my tentative explanation: somewhere, unit tests do go through a code path that depends on generateNetworkConfig
, which is going to check for a binary on the system for the corresponding plugin-type name (GetPluginType
).
For windows, it is looking for "nat", which is only provided by the cni plugins.
For linux, it is looking for "bridge"... which is present on most systems and have nothing to do with CNI - hence we get away with running the unit tests.
I do not think the unit tests themselves actually depend on running the binary (obviously...), but yep, that is making the case that we should look into that code.
Signed-off-by: apostasie <[email protected]>
@@ -213,6 +216,15 @@ func TestParseBuildctlArgsForOCILayout(t *testing.T) { | |||
}, | |||
} | |||
|
|||
if runtime.GOOS == "windows" { | |||
abspath, err := filepath.Abs("/tmp/oci-layout") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be os.Getenv("TEMP")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path /tmp/oci-layout
was / is hardcoded in the oci query part of the test:
nerdctl/pkg/cmd/builder/build_test.go
Lines 206 to 213 in ca76611
ociLayoutPath: "/tmp/oci-layout/", | |
expectedArgs: []string{}, | |
expectedErr: ErrOCILayoutPrefixNotFound.Error(), | |
}, | |
{ | |
name: "DirectoryNotFoundError", | |
ociLayoutName: "unit-test", | |
ociLayoutPath: "oci-layout:///tmp/oci-layout", |
We could change it in both the test declaration and the assert - to GetTemp / Getenv (or whatever) else - but I am not sure it would serve much purpose: it seems this test is solely testing the parsing part of parseBuildContextFromOCILayout
- which does exactly that (filepath.Abs
) after stripping the scheme and will immediately error since there is no oci data in that location.
nerdctl/pkg/cmd/builder/build.go
Line 562 in ca76611
abspath, err := filepath.Abs(path) |
The path could very much be /foo/bar
for all intent and purpose, and I presume who wrote the test did just put /tmp/oci-layout
because that was the first thing they had in mind.
Lmk your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be completely honest, I am not sure this test is bringing much (or any) value, but then, it is here - so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Reopening erroneously closed previous PR.
This one no longer depends on other pending CI changes and can be merged now.
Take-away: