-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add tk diff --exit-zero
flag
#506
Conversation
wdyt @sh0rez? |
cmd/tk/workflow.go
Outdated
@@ -114,6 +114,7 @@ func diffCmd() *cli.Command { | |||
cmd.Flags().StringVar(&opts.Strategy, "diff-strategy", "", "force the diff-strategy to use. Automatically chosen if not set.") | |||
cmd.Flags().BoolVarP(&opts.Summarize, "summarize", "s", false, "print summary of the differences, not the actual contents") | |||
cmd.Flags().BoolVarP(&opts.WithPrune, "with-prune", "p", false, "include objects deleted from the configuration in the differences") | |||
cmd.Flags().BoolVarP(&opts.ExitCode, "exit-code", "", false, fmt.Sprintf("Exit with %d if there were differences and %d if there were no differences.", ExitStatusDiff, ExitStatusClean)) |
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.
A quick check by @Duologic revealed that we have existing scripts that leverage exit status 16
and would break in a subtle way (diffs ignored). Likely other users have these scripts as well.
So I guess we need to keep this switched on by default.
Otherwise LGTM
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.
Fair enough, it was a bit cheeky of me to try to switch the default behaviour here 😉
This does leave us with a dilemma as to what to name the flag. I tend not to like boolean flags that are true by default, because it feels awkward to pass false.
What do you think of --exit-nonzero-only-on-error
, which is rather long, so perhaps -z
for short @sh0rez?
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.
Perhaps if we can't think of a snappy name, having a default-true boolean flag is less bad 🤔
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.
I like -z
! Less long form could be:
-z --exit-zero Exit with 0 even when differences are found
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.
Yeah, that's good I think - I'll push that now.
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.
@sh0rez I've pushed the requested change, and updated the title and description. The current behaviour should be preserved by default.
d8889fb
to
2b3ebeb
Compare
That causes `tk diff` to exit with 0 when there is a diff present. Signed-off-by: Craig Furman <craig.furman89@gmail.com>
2b3ebeb
to
4ff6849
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.
🚀
That causes
tk diff
to exit with 0 when there is a diff present.