Open l0b0 opened 1 year ago
This is a pretty clear issue. Thanks for reporting.
Doing a deep dive on this, there looks to be a deeper problem here.
The actual CLI call into diff is made at https://github.com/aws/aws-cdk/blob/a2a4f6860ce3e5794b443cd810882bbd60b2455f/packages/aws-cdk/lib/cli.ts#L485
This is partly controlled by the feature flag aws-cdk:enabledDiffNoFail https://github.com/aws/aws-cdk/blob/a2a4f6860ce3e5794b443cd810882bbd60b2455f/packages/aws-cdk-lib/cx-api/lib/features.ts#L115
This defaults to 'true'. This gets a weird ternary dance in the configuration of cdk diff
:
fail: args.fail != null ? args.fail : !enableDiffNoFail,
I'm assuming that passing --fail
sets this to some value while --no-fail
sets it to some other value? False? NaN? Undefined? FileNotFound?
I don't know how falsy/truthy null vs other values can be in Typescript, but it appears that the rest of the codebase compares against undefined
rather than null
and this is causing some level of type confusion?
Actually, this flag has a lot of UX issues! I might file separate issues for these, but I'll just make sure to record them here for reference.
Why is a nonzero exit code not the default if diff
finds a difference (reported)? It's literally more information for the end user, and anyone who doesn't care about the exit code could just do || [ $? -eq 1 ]
(except they can't, see below). It's also the default in the most ubiquitous diffing command in existence, diff
itself.
cdk diff --fail
does not have its own exit code (reported). For example:
$ ./node_modules/.bin/cdk diff --fail
[omitted, no diff printed here]
The security token included in the request is expired
$ echo $?
1
Even passing a non-existing flag results in the same exit code (reported):
$ ./node_modules/.bin/cdk diff --wtf
[no mention of the `--wtf` flag here!]
The security token included in the request is expired
This means anyone who wants to react to an actual diff has to parse the output instead of checking the exit code. An exit code of 1 could mean basically anything, making the flag almost completely useless.
--fail
is IMO a terrible name. Any discussion around this flag is basically guaranteed to have a bunch of double negatives. A more explicit flag would be helpful to discover and discuss this feature.
What a crock. It turns out that
npm run
needs a --
between the command and hyphenated options. (Otherwise it presumably parses them as its own?) Younpm run
silently ignores flags it doesn't know about.The end result is that npm run cdk diff --fail
is probably equivalent to npm run --fail cdk diff
. WTF?!
You can see the core of the issue by observing that the command is printed without the flag:
> cdk diff
npm run cdk diff -- --fail
does do the right thing, actually returning a nonzero exit code in case of a difference. It still has terrible UX though, so I'll make sure to report those.
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.
I'm going to leave this open so that we have a tracking issue at least.
This may be a known interaction with npm, but we should at least be aware of it since we do suggest the use of npx
and similar, so this warrants us at least looking into this issue to make sure our customers know about it. Thank you @l0b0 for doing the extensive legwork.
Describe the bug
cdk diff --fail
doesn't do anything, whether it's set totrue
,false
, or nothing.Expected Behavior
--fail
should work exactly likegit diff --exit-code
- if there is a diff, the exit code should be non-zero.Current Behavior
The exit code is always zero, making the flag useless.
Reproduction Steps
See log below.
Possible Solution
No response
Additional Information/Context
No response
CDK CLI Version
2.93.0 (build 724bd01)
Framework Version
No response
Node.js Version
v18.17.1
OS
NixOS
Language
Typescript
Language Version
5.2.2
Other information
Log: