-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
Codecov Report
@@ 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 |
Do we have a good way to add a test for this? |
Yes, it should be possible to unit test this with the fake client |
Design LGTM, moving to code review based on the comments in the previous PR. |
There was a problem hiding this 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?
@dnephin PTAL |
It wouldn't hurt to have a unit test for the new flag since it seems our coverage is low in this area |
888701e
to
f836bda
Compare
looks like a test was added |
Well, it doesn't test anything really.. Having a bit of trouble with |
ping @vdemeester were you still working on a test? |
oh dang it, forgot about that one. |
f836bda
to
4771cbd
Compare
@thaJeztah updated 😉 |
4771cbd
to
de7545d
Compare
@cpuguy83 @thaJeztah Updated, this add a new line to all |
Output (with/without
|
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 |
@docwhat
I don't think it's a concern as those can be used "as is" with the docker commandline (or
Good point, I guess it would be even more useful indeed |
I grok that. But if I pull If I add the namespace and host then Also |
ping @vdemeester I forgot; what's the status on this one? |
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 👍 |
Please rebase @vdemeester 🦁 |
3612063
to
8f6a206
Compare
I'm on the same boat with @jregeimbal , It's very annoying to see the progress output in CI, We must press keycap |
There was a problem hiding this 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 🤗
8f6a206
to
56bdba7
Compare
@thaJeztah updated (and rebased) 😉 |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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)
oh, dang; one test needs updating;
|
There was a problem hiding this 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>
56bdba7
to
dd3407b
Compare
There was a problem hiding this 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!
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
fixes moby/moby#13588
Add
--quiet
to thedocker image pull
subcommand that will pullthe image quietly.
Carrying #553 cc @pramodhkp
🐯
Signed-off-by: Vincent Demeester vincent@sbr.pm