Skip to content

Commit

Permalink
fix(cli): --no-fail flag is ignored in favor of the `enableDiffNoFa…
Browse files Browse the repository at this point in the history
…il` feature flag (aws#21107)

The CLI doesn't distinguish between `cdk diff --no-fail` and just `cdk diff`. In both cases, it's taking the value of `enableDiffNoFail` feature flag to decide which status code to return.

Change the behavior to:

| Is there a difference? | `--fail` (CLI option) | `enableDiffNoFail` (FF) | Return code |
|------------------------|------------------------|-------------------------|-------------|
| No                     | Don't care             | Don't care              | 0           |
| Yes                    | false                  | Don't care              | 0           |
| Yes                    | true                   | Don't care              | 1           |
| Yes                    | null                   | false                   | 1           |
| Yes                    | null                   | true                    | 0           |

Fixes aws#19110.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
otaviomacedo authored Jul 12, 2022
1 parent 6e9963c commit cad6fc5
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 5 deletions.
11 changes: 9 additions & 2 deletions packages/@aws-cdk/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,16 @@
export const ENABLE_STACK_NAME_DUPLICATES_CONTEXT = '@aws-cdk/core:enableStackNameDuplicates';

/**
* IF this is set, `cdk diff` will always exit with 0.
* Determines what status code `cdk diff` should return when the specified stack
* differs from the deployed stack or the local CloudFormation template:
*
* Use `cdk diff --fail` to exit with 1 if there's a diff.
* * aws-cdk:enableDiffNoFail=true => status code == 0
* * aws-cdk:enableDiffNoFail=false => status code == 1
*
* You can override this behavior with the --fail flag:
*
* * --fail => status code == 1
* * --no-fail => status code == 0
*/
export const ENABLE_DIFF_NO_FAIL_CONTEXT = 'aws-cdk:enableDiffNoFail';
/** @deprecated use `ENABLE_DIFF_NO_FAIL_CONTEXT` */
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ async function parseCommandLineArguments() {
.option('template', { type: 'string', desc: 'The path to the CloudFormation template to compare with', requiresArg: true })
.option('strict', { type: 'boolean', desc: 'Do not filter out AWS::CDK::Metadata resources', default: false })
.option('security-only', { type: 'boolean', desc: 'Only diff for broadened security changes', default: false })
.option('fail', { type: 'boolean', desc: 'Fail with exit code 1 in case of diff', default: false }))
.option('fail', { type: 'boolean', desc: 'Fail with exit code 1 in case of diff' }))
.command('metadata [STACK]', 'Returns all metadata associated with this stack')
.command(['acknowledge [ID]', 'ack [ID]'], 'Acknowledge a notice so that it does not show up anymore')
.command('notices', 'Returns a list of relevant notices')
Expand Down Expand Up @@ -416,7 +416,7 @@ async function initCommandLine() {
strict: args.strict,
contextLines: args.contextLines,
securityOnly: args.securityOnly,
fail: args.fail || !enableDiffNoFail,
fail: args.fail != null ? args.fail : !enableDiffNoFail,
stream: args.ci ? process.stdout : undefined,
});

Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk/test/api/deploy-stack.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { deployStack, DeployStackOptions, ToolkitInfo } from '../../lib/api';
import { tryHotswapDeployment } from '../../lib/api/hotswap-deployments';
import { setCI } from '../../lib/logging';
import { DEFAULT_FAKE_TEMPLATE, testStack } from '../util';
import { MockedObject, mockResolvedEnvironment, MockSdk, MockSdkProvider, SyncHandlerSubsetOf } from '../util/mock-sdk';
import { setCI } from '../../lib/logging';

jest.mock('../../lib/api/hotswap-deployments');

Expand Down
37 changes: 37 additions & 0 deletions packages/aws-cdk/test/integ/cli/cli.integtest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,43 @@ integTest('cdk diff', withDefaultFixture(async (fixture) => {
.rejects.toThrow('exited with error');
}));

integTest('enableDiffNoFail', withDefaultFixture(async (fixture) => {
await diffShouldSucceedWith({ fail: false, enableDiffNoFail: false });
await diffShouldSucceedWith({ fail: false, enableDiffNoFail: true });
await diffShouldFailWith({ fail: true, enableDiffNoFail: false });
await diffShouldFailWith({ fail: true, enableDiffNoFail: true });
await diffShouldFailWith({ fail: undefined, enableDiffNoFail: false });
await diffShouldSucceedWith({ fail: undefined, enableDiffNoFail: true });

async function diffShouldSucceedWith(props: DiffParameters) {
await expect(diff(props)).resolves.not.toThrowError();
}

async function diffShouldFailWith(props: DiffParameters) {
await expect(diff(props)).rejects.toThrow('exited with error');
}

async function diff(props: DiffParameters): Promise<string> {
await updateContext(props.enableDiffNoFail);
const flag = props.fail != null
? (props.fail ? '--fail' : '--no-fail')
: '';

return fixture.cdk(['diff', flag, fixture.fullStackName('test-1')]);
}

async function updateContext(enableDiffNoFail: boolean) {
const cdkJson = JSON.parse(await fs.readFile(path.join(fixture.integTestDir, 'cdk.json'), 'utf8'));
cdkJson.context = {
...cdkJson.context,
'aws-cdk:enableDiffNoFail': enableDiffNoFail,
};
await fs.writeFile(path.join(fixture.integTestDir, 'cdk.json'), JSON.stringify(cdkJson));
}

type DiffParameters = { fail?: boolean, enableDiffNoFail: boolean };
}));

integTest('cdk diff --fail on multiple stacks exits with error if any of the stacks contains a diff', withDefaultFixture(async (fixture) => {
// GIVEN
const diff1 = await fixture.cdk(['diff', fixture.fullStackName('test-1')]);
Expand Down

0 comments on commit cad6fc5

Please sign in to comment.