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

Why is this controlled in the client library?

@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.

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.

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

@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 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.

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

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.

I think a CLI only implementation would work well here.

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.

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.

ping @pramodhkp were you still working on this?

@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.

@pramodhkp carrying in #882 😉