-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
c8d integration: skip TestImportWithCustomPlatform #46652
c8d integration: skip TestImportWithCustomPlatform #46652
Conversation
With containerd and its shims I don't think we should be checking the platform any more, for example with this change I wouldn't be able to import a Wasm image any more. Maybe it's best to skip this test? |
Oh, I totally got ahead of myself. Yeah, that makes sense – I'll skip it. However, this goes back to the discussion about whether we want to
Regardless I'll remove the check and skip this particular test as it doesn't make sense. |
ec0ad51
to
27aa4c0
Compare
Do we have any test to test the feature? i.e. to verify that I can import a rootfs, and specify what platform to use for it? |
Because this test-table contains both "successful" ( |
I can split the test out into two (the "successful" cases and the ones that aren't supposed to work, and just skip the "failure" test w/ c8d image store. |
Yes, it's probably worth having some tests for this. Maybe as alternative, we could have an extra field in the test-table, so that we can skip based on that one |
I'd rather split the test and just straight up skip one rather than decide whether to skip dynamically based on an attribute of the test case, for readability. (but if you feel strongly about keeping it as one test I can do that 😅 ) |
Either one is fine |
27aa4c0
to
61e78ad
Compare
@thaJeztah PTAL |
I see this one failing https://github.com/moby/moby/actions/runs/6535667219/job/17745687263?pr=46652, but it's in "test (graphdriver) / integration (ubuntu-20.04, rootless)"; wondering what broke it, or if somehow GitHub actions is reporting results in the wrong place (or running with the wrong options)
|
We support importing images for other platforms when using the containerd image store, so we shouldn't validate the image OS on import. This commit also splits the test into two, so that we can keep running the "success" import with a custom platform tests running w/ c8d while skipping the "error/rejection" test cases. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
61e78ad
to
6f625ae
Compare
Ahh I slipped while copy-pasting 😅 1 sec |
I don't think the remaining failures have to do with this test – @thaJeztah can you TAL? |
LOL, I was trying to find the failing tests in the report (as the logs output was a bit hard to find them all), and then realised they weren't shown as Github was hiding them https://github.com/moby/moby/actions/runs/6536940074 |
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.
LGTM
- What I did
FixTestImportWithCustomPlatform
(failing in https://github.com/moby/moby/actions/runs/6506688442/job/17672826153)Skip failing
TestImportWithCustomPlatform
test cases when using the containerd image store.- How I did it
Reuse the same validation logic from herethat's already used in hereSplit out the "reject" cases from
TestImportWithCustomPlatform
into a new testTestImportWithCustomPlatformReject
, which is skipped when using the containerd image store.- How to verify it
go test -run TestImportWithCustomPlatform ./integration/image/...
go test -run TestImportWithCustomPlatformReject ./integration/image/...
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)