8000 Added option for pulling quietly by pramodhkp · Pull Request #553 · docker/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added option for pulling quietly #553

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Added option for pulling quietly #553

wants to merge 1 commit into from

Conversation

pramodhkp
Copy link
@pramodhkp pramodhkp commented Sep 23, 2017 •
edited
Loading

- What I did
Added an option for quiet docker pulls docker pull -q. Based on the discussion here: moby/moby#13588
- How I did it

- How to verify it

docker pull -q alpine

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@codecov-io
Copy link
codecov-io commented Sep 23, 2017 •
edited
Loading

Codecov Report

Merging #553 into master will decrease coverage by 0.14%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
- Coverage   49.38%   49.23%   -0.15%     
==========================================
  Files         208      200       -8     
  Lines       17178    16455     -723     
==========================================
- Hits         8483     8102     -381     
+ Misses       8262     7933     -329     
+ Partials      433      420      -13

@cpuguy83
Copy link
Collaborator

Why is this controlled in the client library?

@pramodhkp
Copy link
Author

@cpuguy83 The issue is discussed here: moby/moby#13588. Mostly because, piping to dev/null or equivalent would work, but would also suppress error information.

So, having a quiet flag is cleaner.

@dnephin
Copy link
Contributor
dnephin commented Sep 25, 2017

I think the bigger question (which I assume @cpuguy83 was getting at) is why is this even handled in the API? It could be an entirely client-side concern (ignore the bytes sent by pull and only print the relevant errors and final id).

Unfortunately this seems to be the way that docker build -q is implemented as well. Build output can be a lot more verbose so it might make more sense for build. I'm not sure we should do the same for pull. It seems like doing it client-side only would be fine here.

@pramodhkp
Copy link
Author

Maybe. My thoughts were that it would help even when you're making an API call directly to the daemon by skipping the client.

@cpuguy83
Copy link
Collaborator

@dnephin exactly

Shouldn't error information go to stderr?
I'm not intimately familiar with how the pull stream is parsed.
I know build is using the jsonmessage thing which supports sending errors down the stream.

thaJeztah
thaJeztah previously requested changes Sep 26, 2017
View reviewed changes
Copy link
Member
@thaJeztah thaJeztah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

oops, wrong tab 😅

@thaJeztah thaJeztah dismissed their stale review September 26, 2017 13:41

wrong tab LOL

@pramodhkp
Copy link
Author
pramodhkp commented Sep 26, 2017 •
edited
Loading

@thaJeztah Hm, I'll check that. Looks like it's probably because pull_v1.go defines imgID as a string, while pull_v2.go defines imgID as a digest. I'm probably making a mistake while converting.

And yes, I do agree it's a lot of changes for a small feature. But, I'm not sure whether that's a bad thing. 😄

EDIT:
Ah, my bad. I thought I had to return image ID, instead of digest.

@pramodhkp pramodhkp mentioned this pull request Sep 26, 2017
Closed
@pramodhkp
Copy link
Author

@dnephin @cpuguy83 What do you recommend I do with this PR? Modify this to work only on CLI? Or create a new PR?

@dnephin
Copy link
Contributor
dnephin commented Sep 28, 2017

I think implementing it entirely in the client is a good place to start. In the future if we wanted to add it to the API that would still be possible, but I think it's unlikely.

@cpuguy83
Copy link
Collaborator

I think a CLI only implementation would work well here.

@pramodhkp
Copy link
Author

I need some advice implementing this. I'll have to change the DisplayMessagesToStream to ignore the progress messages, but the method is being used in multiple other places. Does it make sense to change everywhere?

Is it possible to send status outside of stream? That way, I can just ignore the stream, and only print status.

@dnephin
Copy link
Contributor
dnephin commented Oct 4, 2017

I would suggest not using DisplayJSONMessagesToStream, most of the functionality there is not relevant for quiet. I think a local function for iterating over the messages, decoding them, and printing just the parts you want would be great.

@pramodhkp
Added option for pulling quietly
eb0374c 
Signed-off-by: Pramodh KP <prmdh1@gmail.com>
@thaJeztah
Copy link
Member

ping @pramodhkp were you still working on this?

@thaJeztah thaJeztah mentioned this pull request Nov 1, 2017
Open
@pramodhkp
Copy link
Author

@thaJeztah I have implemented half of it. But I've been a bit busy. I will create a new PR when I'm done, if this needs to be closed.

@thaJeztah thaJeztah mentioned this pull request Dec 21, 2017
Closed
@docker docker deleted a comment from GordonTheTurtle Feb 16, 2018
@vdemeester
Copy link
Collaborator

@pramodhkp carrying in #882 😉

@vdemeester vdemeester closed this Feb 16, 2018
@vdemeester vdemeester mentioned this pull request Feb 16, 2018
Merged
thaJeztah added a commit that referenced this pull request Dec 19, 2018
@thaJeztah
Merge pull request #882 from vdemeester/carry-553-pull-quiet-cli
6deb4f1 
Carry #553 : Add option to pull images quietly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah left review com 57AE ments

Assignees
No one assigned
Labels
status/1-design-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pramodhkp @codecov-io @cpuguy83 @dnephin @thaJeztah @vdemeester @GordonTheTurtle
0