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

Add support for multiple image on docker push #15624

Closed
wants to merge 1 commit into from

Conversation

vdemeester
Copy link
Member

Fixes #7336 : makes it possible to push multiple images at once (and thus multiple tags without pushing all of them).

It groups tags for the same repository and sends a request to the daemon for each repository with the tags. This does not change the way images are pushed.

$ docker push vdemeester/image:v1 vdemeester/image:v2 vdemeester/another-image
# - (first) asks the daemon to push vdemeester/image with tags v1 & v2 only (not all tags)
# - (then) asks the daemon to push vdemeester/another-image (all tags)

The push(es) are done sequentially (not in parallel).

If there is an error while pushing, it's handled the same way as docker rm commands (or rmi, …) : it will log out the error pushes and print a message at the end, that should look like : Err: failed to push images: [errorimage:[] another:[v1 v2]].

  • I don't like too much the error message at the end (especially the :[] part), but it's purely aesthetics so it can wait a bit after design review.
  • API & documentation will have to be update but it can wait for docs review.

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

- it's now possible to push multiple tag "at once" for a repository ;
- previously it was one or all of them.
- the push(es) are done sequentially (not in parallel).

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@thaJeztah
Copy link
Member

ping @stevvooe @dmcgowan for (design) review. This is a feature-request that keeps popping up :-)

@dmcgowan
Copy link
Member

Indeed this is a feature which keeps popping up. Last time I recall was adding this on push and we rejected the design because it did not add any benefits over doing it as a for loop, but it did keep us from making other changes to the positional arguments on push and pull. See #11682

We can certainly re-asses its value but would want the feature to include a real optimized use case rather than simple cli sugar. Otherwise we could discuss dismissing the change @sday mentioned as something that will never happen.

@vdemeester
Copy link
Member Author

@dmcgowan Yeah that's almost the same as the docker pull propositon on #11682 but for push this time. The benefit/optimization here, is that if we want to push 3 tags of the same repository it will ask the daemon once (once per repository, no matter how much tag there is) — in cli and using the API. It's also a little bit of an UX problems : rmi took any number of arguments but not push and pull, which can feel weird sometimes 😅.
The optimization could go further by sending repository & tags all at once to the daemon and let him handle it but it changes the API way more.

That being said, reading the #11682 comments, I do agree that it will block potential positional arguments on push ; and that would be a pain to make something retro-compatible. So if the git remote syntax is still in thinking the PR should be closed (but this means we should comment and close #7336 too). 😊

@stevvooe
Copy link
Contributor

@vdemeester @dmcgowan I am against adding this functionality without first addressing the major bugs in this section of the code and deciding the future solution to remotes. Specifically, we don't want to extend the push/pull command syntax until we've actually locked down image reference syntax and have made decision about how one might select push location.

Also, it may actually make more sense to go to a job-based model, which would allow one to issue several pushes, but have the command exit right away, allowing the operation to run in the background.

@vdemeester
Copy link
Member Author

@dmcgowan @stevvooe Thanks for the information (and for the time to write it 😛). It's clear to me now that this is not the right time to do this change for several reasons.

[…] without first addressing the major bugs in this section of the code and deciding the future solution to remotes.

I completely agree with that ; fixing the bug/actual code is more important than add a new functionnality. So I'm gonna close this one for now 😉 .

@vdemeester vdemeester closed this Aug 18, 2015
@stevvooe
Copy link
Contributor

@vdemeester Thanks for understanding!

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

Successfully merging this pull request may close these issues.

None yet

5 participants