Update deploy to have output during dry-run too #119
Conversation
| dryRunOut <- j | ||
|
|
||
| // TODO this isn't exactly *failure* -- maybe we should just add a logText suffix instead? | ||
| fmt.Fprintln(os.Stderr, failurePrefix+logText) |
There was a problem hiding this comment.
A good argument in favor of dropping the TODO here and just accepting that this is failure is that this case is basically "we do need to push, but were blocked by configuration"
There was a problem hiding this comment.
I agree that an unpushed image should be considered a failure on dry-run. Should this also exit at the end with an error if there were things needing to be pushed when doing dry-run?
There was a problem hiding this comment.
That's a harder question -- I think it's more useful/easier to use if "non-empty stdout" means something needed to be pushed, since the whole point is querying what did need to be pushed, and a non-zero exit code then makes it harder to differentiate breaking errors vs just "things need to be pushed".
This moves the "normal" deploy output from stdout to stderr so that we can print it all the time (whether it's a dry-run or not), such that dry-run can have the same progress output. Without this, "dry-run" doesn't print any status, especially in the case of 100% "already pushed" (which is the whole reason we implemented dry-run in the first place, so it'd helpful for it to have *some* output).
2 participants
This moves the "normal" deploy output from stdout to stderr so that we can print it all the time (whether it's a dry-run or not), such that dry-run can have the same progress output.
For now, I've decided that "needs-push" means "failure" and printed the status accordingly, but I could be convinced it should do something else instead.(#119 (comment))Without this, "dry-run" doesn't print any status, especially in the case of 100% "already pushed" (which is the whole reason we implemented dry-run in the first place, so it'd helpful for it to have some output).