Skip to content

Conversation

craigloewen-msft
Copy link
Contributor

I made a PR to add the --export command to nerdctl.

I did it to solve issues like #1854 and since I saw it had support from the maintainers.

I also looked at this PR for inspiration: https://github.com/containerd/nerdctl/pull/2161/files

Please let me know if you want any changes or if it looks good!

@craigloewen-msft
Copy link
Contributor Author

@AkihiroSuda is this change good to go? Anything else I can do to help merge this in? :)

@AkihiroSuda
Copy link
Member

Not compilable 😞

https://github.com/containerd/nerdctl/actions/runs/16174086400/job/45654760202?pr=4405

# github.com/containerd/nerdctl/v2/pkg/cmd/container
Error: pkg/cmd/container/export.go:69:38: undefined: containerutil.MountSnapshotForContainer

@craigloewen-msft
Copy link
Contributor Author

Not compilable 😞

https://github.com/containerd/nerdctl/actions/runs/16174086400/job/45654760202?pr=4405

# github.com/containerd/nerdctl/v2/pkg/cmd/container
Error: pkg/cmd/container/export.go:69:38: undefined: containerutil.MountSnapshotForContainer

Fixed the build!

@craigloewen-msft
Copy link
Contributor Author

@AkihiroSuda I fixed the PR!
Looks like all the linting succeeds now, it's using the correct WriteDiff function and the only failing tests seem to be from other causes.

@apostasie
Copy link
Contributor

@craigloewen-msft
Copy link
Contributor Author

@apostasie thank you for the spot. Seems like somehow it's failing on Windows? Would need to dig into that further but for now I have to pause for a little to prioritize my other work.

@craigloewen-msft
Copy link
Contributor Author

Errors seem to be an access denied error and a can't access the file since it's already in use. Maybe my use of the snapshotter here is bad??

@craigloewen-msft
Copy link
Contributor Author

I did investigation and docker just bans the exports of Windows containers. I think we should follow suite. @AkihiroSuda does this seem good to you? If so I can just add in an error saying export is not allowed on Windows.

@AkihiroSuda
Copy link
Member

I did investigation and docker just bans the exports of Windows containers. I think we should follow suite

👍

@craigloewen-msft
Copy link
Contributor Author

It seems like it passes all the tests now! I looked at the failures and they seem to be related to other issues. Is this ready to merge? Anything else I can fix?

@AkihiroSuda
Copy link
Member

Please squash the commits

@AkihiroSuda AkihiroSuda added this to the v2.1.4 milestone Aug 1, 2025
@craigloewen-msft
Copy link
Contributor Author

Please squash the commits

Do you want me to squash all the commits on my fork? I could do that but it would break my GitHub history. Can you just squash it when you merge it in here using the 'squash commits' tool in the GitHub PR review?

If not, I'm happy to squash my commits and force a push to my main branch.

@AkihiroSuda
Copy link
Member

Do you want me to squash all the commits on my fork?

Yes.

I could do that but it would break my GitHub history.

You can back up the history in your repo using git tags

Can you just squash it when you merge it in here using the 'squash commits' tool in the GitHub PR review?

We have been expecting contributors to tidy up the commit messages, so the "squash commits" button does not work for us.

CI is also failing
https://github.com/containerd/nerdctl/actions/runs/16732676050/job/47364238814?pr=4405

FAIL - does not have a valid DCO

Signed-off-by: Craig Loewen <[email protected]>
@craigloewen-msft
Copy link
Contributor Author

Tests all pass! The only failure is due to some other test failing.
image

Seems like we should be good to merge now :)

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

@AkihiroSuda AkihiroSuda merged commit 15f1649 into containerd:main Aug 18, 2025
93 of 98 checks passed
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.

3 participants