cdk diff: always fails on diff

[rittneje]

What is the problem?

The docs for cdk diff with respect to --fail are wrong.

  --fail                Fail with exit code 1 in case of diff
                                                 [boolean] [default: false]

1. By default cdk diff exits with status code 1 if there is a diff.
2. Neither --fail=false nor --no-fail have any effect.

In a deployment pipeline, we do not want the build to fail when there is a diff. But we also don't want to ignore the exit code from cdk diff entirely, since if it fails for some other reason that should be fatal.

Reproduction Steps

Run cdk diff.

What did you expect to happen?

I expected cdk diff to abide by the documented behavior.

What actually happened?

It failed in violation of the documented behavior.

CDK CLI Version

1.140.0

Framework Version

No response

Node.js Version

16.13.2

OS

Alpine 3.15

Language

Python

Language Version

3.9.10

Other information

No response

[ryparker]

Hey @rittneje 👋🏻

I wasn't able to reproduce this behavior with v2.13.0 (build b0b744d). Would you mind testing this with the latest CDK version?

cdk diff -> exit code 0
cdk diff --fail true -> exit code 1
cdk diff --fail false -> exit code 0

[rittneje]

@ryparker This is the source of the bug in v1.

aws-cdk/packages/aws-cdk/bin/cdk.ts

Line 333 in 789f2dc

fail: args.fail || !enableDiffNoFail,

Since args.fail is itself a boolean, when we pass --no-fail, it incorrectly causes it to fall back to the aws-cdk:enableDiffNoFail feature flag, which itself defaults to false. Ergo, passing --no-fail has no effect whatsoever, with or without that feature flag.

I strongly question why that feature flag was even introduced, instead of just letting people opt-out via --no-fail.

In any case, please fix v1 to handle the boolean correctly. It needs to distinguish null (as in no flag was passed, so use the default) from false (as in --no-fail was passed explicitly). Also, please fix the cdk docs to make it clear that the default value of this flag is contingent upon the aws-cdk:enableDiffNoFail feature flag in v1. https://docs.aws.amazon.com/cdk/v1/guide/cli.html#cli-ref

[TheRealAmazonKendra]

@rittneje The same line of code exists for v1 and v2. When you have the feature flag enabled in v1 it produces the behavior @ryparker displayed above. If the app you're experiencing this issue in was created before the feature flag existed, you'll need to add it to the context in cdk.json. From what I can tell, this is working as intended.

[rittneje]

@TheRealAmazonKendra If I explicitly pass --fail or --no-fail at the command line, then the feature flag should not matter. However, there is currently a bug where passing --no-fail actually means to do whatever the feature flag says. This is true in both v1 and v2. The only difference is that in v2 the feature flag is enabled by default. Frankly, I feel it was a design flaw to use a feature flag for this, because clients can just as easily pass one of those two command line flags. (If anything, it should have been a first-class field in cdk.json a la versionReporting, not a context attribute.)

[TheRealAmazonKendra]

The feature flag was used because it caused a breaking change when it was implemented. You are correct that it is enabled by default in v2. The fix here, however, is to enable the feature flag in your project and then it will work by passing those very cli arguments. I understand the frustration of "it should have been this way in the first place," but because it wasn't, and the fix would cause a breaking change for customers, the feature flag was added.

[rittneje]

@TheRealAmazonKendra You misunderstand. Originally cdk diff always exited with failure if there was a diff. Many people (myself included) found this behavior undesirable, and wanted a way to make it always exit successfully. Adding support for the --no-fail (or equivalently --fail=false) command line flag alone would have addressed this. There was no need to introduce a feature flag, because passing the command line flag itself is an explicit opt-in. However, the --no-fail flag was itself implemented incorrectly, such that it has no effect at all. That bug needs to be fixed.

What the feature flag should have done is changed the default behavior of cdk diff. Instead, it also kind of "enabled" the ability to use the --fail command line flag, which doesn't really make any sense. What should have been done is the following:

1. In CDK v1, the default behavior of cdk diff remains to fail on diff.
2. In CDK v2, the default behavior of cdk diff is not to fail on diff.
3. In cdk.json, I can optionally specify "failOnDiff": true|false to override the default behavior in either version. (This should NOT be a feature flag since it isn't enabling or disabling a "feature" as such.)
4. At the command line, I can explicitly pass --fail or --no-fail to override any default behavior in either version.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.