Skip to content

Added --export command #4405

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

craigloewen-msft
Copy link

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
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
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
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
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
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
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
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
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
Author

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

Seems like we should be good to merge now :)

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