Skip to content

Allow progress also when saving to docker daemon #972

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow progress also when saving to docker daemon #972

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/v1/daemon/write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,15 @@ func Tag(src, dest name.Tag) error {
}

// Write saves the image into the daemon as the given tag.
func Write(tag name.Tag, img v1.Image) (string, error) {
func Write(tag name.Tag, img v1.Image, opts ...tarball.WriteOption) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit weird for daemon.Write to take a tarball.Options, it leaks a bit of the underlying abstraction.

Would it make sense to define a daemon Option for progress and convert it internally into the tarball Option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that remote.Write has the same problem, due to WriteOptions being added on the wrong level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the right thing to do would probably be to add a daemon.WithProgress, instead of tarball.WithProgress

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah exactly. #967 defines a new remote.WithProgress. In the case of daemon, it might end up using tarball.WithProgress underneath, but it feels weird to have that leak into its API.

There isn't a daemon.WriteOption type yet, but I think we could add one, and a daemon.WithProgress that implements it, that converts to a tarball.WriteOption internally.

@jonjohnsonjr wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise it seems to be working, of course we might end up using tarball anyway for Podman support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've stomped on this PR a little bit by making everything just daemon.Option (sorry), but it shouldn't be too hard to do this now by just translating the daemon.WithProgress into a tarball.WithProgress.

Copy link
Contributor Author

@afbjorklund afbjorklund Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had to fork everything anyway, to make a podman matching the daemon in the API.

https://github.com/kubernetes/minikube/pull/11063/files#diff-fa213d52c815c8c5520c1a6564d404ecf1d5ea5f3acf11dfd86a406bab4a4d3a

cli, err := GetImageLoader()
if err != nil {
return "", err
}

pr, pw := io.Pipe()
go func() {
pw.CloseWithError(tarball.Write(tag, img, pw))
pw.CloseWithError(tarball.Write(tag, img, pw, opts...))
}()

// write the image in docker save format first, then load it
Expand Down