Skip to content
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

image: dedup same platform entry #2486

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Conversation

zhuangqh
Copy link
Contributor

may got duplicate value from image.Platforms interface.
we should duplicate them like ctr do

Signed-off-by: zhuangqh zhuangqhc@gmail.com

@AkihiroSuda
Copy link
Member

How to repro the issue?

@zhuangqh
Copy link
Contributor Author

How to repro the issue?

when listing image which was generated from criu checkpoint. see containerd/containerd#9081

@AkihiroSuda
Copy link
Member

Windows test is failing

=== CONT  TestImagesWithNames
    image_list_test.go:36: assertion failed: 
        Command:  C:\Users\runneradmin\go\bin\nerdctl.exe --namespace=nerdctl-test images --names gcr.io/k8s-staging-e2e-test-images/busybox:1.29-2
        ExitCode: 0
        Stdout:   NAME    IMAGE ID    CREATED    PLATFORM    SIZE    BLOB SIZE
        
        Stderr:   
        
        Failures:
        Expected stdout to contain "gcr.io/k8s-staging-e2e-test-images/busybox:1.29-2"
--- FAIL: TestImagesWithNames (0.55s)

https://github.com/containerd/nerdctl/actions/runs/6146430040/job/16700918243?pr=2486

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@zhuangqh
Copy link
Contributor Author

@AkihiroSuda I have no idea whether to dudup inside the image.Platform() implementation or here. What do you recommend

@AkihiroSuda
Copy link
Member

@AkihiroSuda I have no idea whether to dudup inside the image.Platform() implementation or here. What do you recommend

Ideally deduplication should happen in image.Platform()

@zhuangqh
Copy link
Contributor Author

ok, I'm committing changes to the upstream containerd repository. So hold this pr, thanks

@zhuangqh
Copy link
Contributor Author

Still dedup here as a workaround, see containerd/containerd#9081 (comment)
thanks @AkihiroSuda

@AkihiroSuda AkihiroSuda added this to the v1.7.0 (tentative) milestone Sep 27, 2023
@AkihiroSuda AkihiroSuda merged commit 9fa8b19 into containerd:main Sep 27, 2023
21 checks passed
@AkihiroSuda
Copy link
Member

Thanks, merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants