Fix panics when --compress and --stream are used together#1105
Fix panics when --compress and --stream are used together#1105vdemeester merged 1 commit intodocker:masterfrom
Conversation
vdemeester
requested review from
dnephin,
silvin-lubecki,
thaJeztah,
tonistiigi and
vieux
cli/command/image/build.go
Outdated
| ) | ||
|
|
||
| if options.compress && options.stream { | ||
| fmt.Fprintf(dockerCli.Err(), "--compress has no effect when used with --stream") |
There was a problem hiding this comment.
nit: "--compress is ignored when used with --stream" ?
There was a problem hiding this comment.
nitnit: or "--compress ignored when used with --stream"?
There was a problem hiding this comment.
Still wondering if we should make this a hard error, e.g. we do so here;
cli/cli/command/service/update.go
Line 1105 in 57ce5aa
There was a problem hiding this comment.
@thaJeztah my take on this is : --stream is experimental still, and it could make sense that we would like to compress the build stream in the future, that's why I didn't want to error out 👼
There was a problem hiding this comment.
Right; so in that case it's even better to error out, because otherwise the option is ignored now (so people can keep using it, it has no effect), but when we implement compression for streaming, the behavior changes, and now the flag does have effect.
Producing an error now, makes sure that nobody uses it
There was a problem hiding this comment.
alright, seems fair then 👼
cli/command/image/build.go
Outdated
| } | ||
|
|
||
| if options.compress { | ||
| if options.compress && !options.stream { |
There was a problem hiding this comment.
Add a comment to explain why compress is ignored?
Warns that `-compress` has no effect when used together with the expremintal `--stream` flag. Signed-off-by: Vincent Demeester <vincent@sbr.pm>
vdemeester
force-pushed
the
1044-compress-stream-friend
branch
from
ceabf98 to
8b3dc39
Compare
|
Updated with erroring out 👼 |
6 participants
Warns that
-compresshas no effect when used together with theexpremintal
--streamflag.Fixes #1044
Fixes moby/moby#36579
cc @dgageot
Signed-off-by: Vincent Demeester vincent@sbr.pm