diff: Add --no-error-on-diff flag

[camh-]

Add a --no-error-on-diff flag to the diff subcommand so as to not
exit with an error when there are diffs present. It will still exit with
an error if the configs are not valid, so kubecfg diff can be a useful
command to run on a branch of a CI system to show diffs and still
validate the configs.

[camh-]

Not sure if I should have raised an issue to discuss this feature first, but this is the third time I've needed this (with a TODO in my Makefiles to do it). I find this feature useful as I can run kubecfg diff on a branch and CI shows the diffs and validates the configs, and on master it is deployed. Because I run everything from a Makefile, it is a bit hard to detect the exit(10) that you get for a difference present condition.

[camh-]

Not sure how to get a passing build. Travis is saying:

$ travis_setup_go
gimme: given '1.12.x' but no release for '1.12' found
gimme: given '1.12.x' but no release for '1.12' found
Failed to run gimme

[mkmik]

that's weird, I ran this 3 days ago and it worked: https://travis-ci.org/bitnami/kubecfg/jobs/648533819?utm_medium=notification&utm_source=github_status

probably a transient issue, restarting the job

[mkmik]

Not sure kubecfg needs to have a special feature of that.
What's wrong with just using the standard shell's features to deal with that, like:

kubecfg diff .... || true

?

[camh-]

What's wrong with just using the standard shell's features to deal with that, like:

I covered that partly in the commit message - that shell construct will hide errors that are not due to a diff. But the other reason is that you may not have a shell because you have kubecfg in a distroless image or similar. Also I avoid running things with the shell as much as possible because it introduces quoting problems and changes argv from a vector to a single string (sh -c "single string").

But the other reason I'd like this patch to be merged is I don't think kubecfg doing exactly what you ask it to do should result in an error. At least how I use it. I think it should be the default not to exit with an error but it's too late for that.

[mkmik]

but it's too late for that.

I wonder how many people are actually depending on that behavior.

I'm all for not breaking backward compatibility, but what if a behavior is objectively wrong? That would be a bug and bugs should be fixed, right?

What does kubectl diff do?

[camh-]

What does kubectl diff do?

I'm not sure. I haven't used that. And I can't run it now because we have "security" in my cluster:

$ kubectl diff -f foo.yaml
Error from server (BadRequest): admission webhook "validation.gatekeeper.sh" does not support dry run

As for whether bugs should be fixed that change behaviour, i think it comes down to pragmatism. If people have built scripts that depend on this behaviour, you'll be breaking them if you change this. I can see kubecfg diff being used as a validation step to ensure things have not changed that should not be changed - such validation could now pass unintentionally if you changed the default exit code to be 0.

[shric]

kubectl diff returns 0 on no difference and 1 on a difference and 1 on some other error (e.g. no such yaml file)
diff returns 0 on no difference and 1 on a difference and 2 on some other error.
I think that both kubecfg diff and kubectl diff should behave the same as diff so that you can discern error from difference or check != 0 if you don't care.

[mkmik]

Makes sense.

So, there are two main reasons why one would want to run k* diff:

1. To log what will change when we apply/update.
2. To detect config drift (e.g. to ensure that what you have checked in matches what is running in the cluster)

If I understood correctly, the current behaviour (consistent with kubectl and good old unix diff) works well out of the box in the second use case but poorly in the first use case, because if all you do is to look whether the exit code is non 0 you may misinterpret unrelated failures (e.g. transient networking, configuration) as "there are differences".

Technically, the exit code can encode such difference, by using the value 1 for "there are differences" and 2 for actual errors, as unix diff and grep utilities do (Not sure if kubecfg already does that, consistently. I think it should). But that means that users of kubecfg diff would need to have a more complicated exit status check, e.g. when using it in shell scripts they should use $? (something they should so anyways when writing robust scripts anyway even when using diff and grepvand friends, but rarely people does so, probably because it's cumbersome).

What if instead of adding a flag that forces found differences to be reported as exit code 0, we explicitly address the first use case and add a --diff flag to kubecfg update ?

Perhaps to could even ask for interactive confirmation before continuing (overrideable with --yes).

(It's been a long time since the last time I used borgcfg ; I think it worked that way although I forgot the details )

---

$ kubecfg update --diff foo.jsonnet
<some yaml diff shown>
Update? [Yn]:

[camh-]

What if instead of adding a flag that forces found differences to be reported as exit code 0, we explicitly address the first use case and add a --diff flag to kubecfg update ?

That would work for me - in my CI/CD I do the diff followed by an update which is either a dry run on a branch or not if master. That would probably shave some seconds off my CI.

Perhaps to could even ask for interactive confirmation before continuing (overrideable with --yes).

As long as it can be overridden, I'm fine with that. I don't know whether it makes more sense to have -i/--interactive to add the prompt or --yes to remove it - if I'm asking kubecfg to apply an update, the intent is clear that I want it done. Does --diff change that intent more often that not?

We would also need to add the --diff-strategy flag too - I always use it with subset (I'm not sure I understand the use case for all - it seems to me to always show diffs that mean little to me).

[mkmik]

I don't have strong feelings for whether to default to interactive or not. On one hand I do agree with you that "do what I tell you" would more naturally follow the principle of least surprise. On the other hand I don't know of there are some precedents set by sister tools.

As for diff strategy: I agree. I always wondered the same. @anguslees?

[shric]

We would also need to add the --diff-strategy flag too - I always use it with subset (I'm not sure I understand the use case for all - it seems to me to always show diffs that mean little to me).

The --diff-strategy subset has some deficiencies, for example #228 , as update uses the last-applied-configuration annotation.

These leads me to the concern that if update --diff is implemented, I will always want to use that instead of diff. It will also be confusing if the diff output of update --diff differs from diff. This would be addressed if/when #228 (comment) is done. I will comment on that issue as well.