Fix stdout and errors in image/trust#636
Conversation
dnephin
force-pushed
the
fix-trust-output-stream
branch
from
33e55e9 to
78c5719
Compare
|
|
||
| if target == nil { | ||
| fmt.Fprintln(streams.Out(), "No targets found, please provide a specific tag in order to sign it") | ||
| return nil |
There was a problem hiding this comment.
Note this is now an error, so the cli will exit with non-zero. Since the push was not actually completed it seems like this should be an error.
Codecov Report
@@ Coverage Diff @@
## master #636 +/- ##
==========================================
+ Coverage 49.4% 49.41% +0.01%
==========================================
Files 208 208
Lines 17190 17186 -4
==========================================
+ Hits 8492 8493 +1
+ Misses 8265 8260 -5
Partials 433 433 |
dnephin
force-pushed
the
fix-trust-output-stream
branch
from
78c5719 to
8f86a25
Compare
Signed-off-by: Daniel Nephin <dnephin@docker.com>
| trustedFamiliarRef := reference.FamiliarString(trustedRef) | ||
|
|
||
| fmt.Fprintf(cli.Out(), "Tagging %s as %s\n", trustedFamiliarRef, familiarRef) | ||
| fmt.Fprintf(cli.Err(), "Tagging %s as %s\n", trustedFamiliarRef, familiarRef) |
There was a problem hiding this comment.
Note sure whether this should go to stderr. This is the success message.
There was a problem hiding this comment.
This is the fix for the issue raised in #631.
The convention we should follow for stdout vs stderr error is:
stdoutis for "normal program output"stderris for everything else (logging, warnings, etc)
This function is called from 3 commands. For both image build and container create this is definitely not normal program output, it's logging, so stderr is definitely correct.
For the last (image pull) it is a little less clear, but I think it could still easily be considered logging, since the desired action was "pull", not "tag".
There was a problem hiding this comment.
I guess that depends on whether one expects command "X" to be different to command "X with trust." In my mind, we should send some minimal amount of info to stdout such that if I pipe stderr to /dev/null I can still determine the operation behaved as desired.
There was a problem hiding this comment.
This is not how posix programs are supported to work. If we send anything to stdout we break anyone trying to use docker build and docker run in a script, when used with trust. The only output to stdout should be normal program output, which is the output from the command being run by docker run.
There is already a mechanism that allows you to determine if the operation behaved correctly or not, the exit status code. This PR also fixes some problems where the exit status was not being used correctly.
7 participants
Fixes #631
Move some output to stderr.
Moved some error output into the error message instead of as a separate print.
Changed the behaviour in some cases where the conenction to notary failed to error instead of exit without an error.