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

Carry #553 : Add option to pull images quietly #882

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

vdemeester
Copy link
Collaborator

@vdemeester vdemeester commented Feb 16, 2018

fixes moby/moby#13588

Add --quiet to the docker image pull subcommand that will pull
the image quietly.

$ docker pull -q golang:latest
$ docker pull -q golang
Using default tag: latest

Carrying #553 cc @pramodhkp

🐯

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

@codecov-io
Copy link

codecov-io commented Feb 16, 2018

Codecov Report

Merging #882 into master will decrease coverage by 0.01%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
- Coverage   55.26%   55.25%   -0.02%     
==========================================
  Files         289      289              
  Lines       19385    19395      +10     
==========================================
+ Hits        10713    10716       +3     
- Misses       7977     7983       +6     
- Partials      695      696       +1

@cpuguy83
Copy link
Collaborator

Do we have a good way to add a test for this?

@dnephin
Copy link
Contributor

dnephin commented Feb 16, 2018

Yes, it should be possible to unit test this with the fake client

@dnephin
Copy link
Contributor

dnephin commented Feb 16, 2018

Design LGTM, moving to code review based on the comments in the previous PR.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

changes LGTM. This also needs changes to the completion scripts; do you want to take care of those in this PR, or in a follow up?

@thaJeztah
Copy link
Member

@dnephin PTAL

@dnephin
Copy link
Contributor

dnephin commented Feb 19, 2018

It wouldn't hurt to have a unit test for the new flag since it seems our coverage is low in this area

@thaJeztah
Copy link
Member

looks like a test was added

@vdemeester
Copy link
Collaborator Author

vdemeester commented Feb 22, 2018

looks like a test was added

Well, it doesn't test anything really.. Having a bit of trouble with jsonmessage 😝 😅

@thaJeztah
Copy link
Member

ping @vdemeester were you still working on a test?

@vdemeester
Copy link
Collaborator Author

oh dang it, forgot about that one.

@vdemeester
Copy link
Collaborator Author

@thaJeztah updated 😉

e2e/image/pull_test.go Outdated Show resolved Hide resolved
@vdemeester vdemeester removed the request for review from dnephin April 20, 2018 13:58
@vdemeester
Copy link
Collaborator Author

@cpuguy83 @thaJeztah Updated, this add a new line to all docker pull like that.

@thaJeztah
Copy link
Member

Output (with/without -q) now looks like:

docker pull nginx:alpine
alpine: Pulling from library/nginx
Digest: sha256:3a44395131c5a9704417d19ab4c8d6cb104013659f5babb2f1c632e789588196
Status: Image is up to date for nginx:alpine
docker.io/library/nginx:alpine
docker pull -q nginx:alpine
docker.io/library/nginx:alpine
  • is it a concern that we output the fully-qualified name (with docker.io...)?
  • would the digest be useful? i.e.; docker.io/library/nginx:alpine@sha256:3a44395131c5a9704417d19ab4c8d6cb104013659f5babb2f1c632e789588196 (or docker.io/library/nginx@sha256:3a44395131c5a9704417d19ab4c8d6cb104013659f5babb2f1c632e789588196 - without tag)?

@docwhat
Copy link

docwhat commented May 17, 2018

The :tag@sha form would be helpful. It would have everything you could want in a compact reasonably computer parseable way.

The docker.io should go away if it doesn’t match the image name. That both docker pull and docker run should be able to use the returned string.

@vdemeester
Copy link
Collaborator Author

vdemeester commented May 18, 2018

The docker.io should go away if it doesn’t match the image name. That both docker pull and docker run should be able to use the returned string.

@docwhat docker run and docker pull are able to run docker.io/library/nginx:alpine (it's just that docker has some default — hub — and special cases — library).

is it a concern that we output the fully-qualified name (with docker.io...)?

I don't think it's a concern as those can be used "as is" with the docker commandline (or containerd, …)

would the digest be useful?

Good point, I guess it would be even more useful indeed

@docwhat
Copy link

docwhat commented May 18, 2018

I grok that. But if I pull nginx then I can run nginx.

If I add the namespace and host then docker run will go pull it again (though it’ll be fast since all the layers are already here).

Also docker image ls will show nginx without the namespace and host.

@thaJeztah
Copy link
Member

ping @vdemeester I forgot; what's the status on this one?

@jregeimbal
Copy link

Bump, I've spent hours searching for a viable workaround to quiet down docker pull/push. Nothing seems to work with Semaphore CI. This would be a great feature to get in soon 👍

@silvin-lubecki
Copy link
Contributor

Please rebase @vdemeester 🦁

@Gasol
Copy link

Gasol commented Dec 14, 2018

I'm on the same boat with @jregeimbal , It's very annoying to see the progress output in CI, We must press keycap page down twice or even more to ignore them over and over.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

whoops, missed that this was rebased; found one issue 🤗

e2e/image/pull_test.go Outdated Show resolved Hide resolved
@vdemeester
Copy link
Collaborator Author

@thaJeztah updated (and rebased) 😉

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!

@@ -202,7 +203,12 @@ func trustedPull(ctx context.Context, cli command.Cli, imgRefAndAuth trust.Image
if err != nil {
return err
}
if err := imagePullPrivileged(ctx, cli, updatedImgRefAndAuth, false, platform); err != nil {
if err := imagePullPrivileged(ctx, cli, updatedImgRefAndAuth, PullOptions{
all: false,
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but wondering if we need a validation step for --all somewhere (as it's being ignored here); i.e. is there a situation where I could've provided the --all flag, and no error is printed (but it's being ignored)

@thaJeztah
Copy link
Member

oh, dang; one test needs updating;

--- FAIL: TestNewPullCommandSuccess (0.00s)
    pull_test.go:87: assertion failed: 
        --- expected
        +++ actual
        @@ -1,2 +1,3 @@
        +Using default tag: latest
         docker.io/library/image:latest
         

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

see above; LGTM after the test was updated 😅 😇

Add `--quiet` to the `docker image pull` subcommand that will not pull
the image quietly.

```
$ docker pull -q golang
Using default tag: latest
```

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

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing!

@thaJeztah thaJeztah merged commit 6deb4f1 into docker:master Dec 19, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Dec 19, 2018
@vdemeester vdemeester deleted the carry-553-pull-quiet-cli branch December 19, 2018 13:23
facebook-github-bot pushed a commit to pytorch/pytorch that referenced this pull request Jul 20, 2019
Summary:
I couldn't find any verbosity options in the [`docker pull` command docs](https://docs.docker.com/engine/reference/commandline/pull/), but
`docker pull` [got a `--quiet` option](docker/cli#882) in a recent version (not sure if we're using that version), and `--quiet` for `docker push` [is forthcoming](docker/cli#1221).
Pull Request resolved: #23111

Differential Revision: D16402993

Pulled By: kostmo

fbshipit-source-id: 52f77b11b839d28f8cf1ecb58518ca69632d7fbe
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.

Add a quiet option to docker pull