fix(cdk): cdk diff --quiet to print stack name when there is diffs#30186
fix(cdk): cdk diff --quiet to print stack name when there is diffs#30186mergify[bot] merged 16 commits intoaws:mainfrom
cdk diff --quiet to print stack name when there is diffs#30186Conversation
| // WHEN | ||
| const exitCode = await toolkit.diff({ | ||
| stackNames: ['A', 'A'], | ||
| stackNames: ['D'], |
There was a problem hiding this comment.
I fixed a broken test. Originally, the test assumed there was no difference, but the specification stackNames: ['A', 'A'] actually implied a difference existed. The assertion not.toContain passed due to the presence or absence of ANSI escape sequences. To correct this, I changed the specification to stackNames: ['D'], where no difference exists, and I also removed the ANSI escape sequences from the assertion string.
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Exemption Request: I think the unit tests cover the test for this change. |
packages/aws-cdk/lib/cdk-toolkit.ts
Outdated
| } | ||
| } | ||
|
|
||
| const diff = options.securityOnly |
There was a problem hiding this comment.
In the previous PR, it was commented that the stack name output should be done in the printSecurityDiff function, but I decided to write it this way because we think it is more user-friendly to write all messages after the stack name output.
Please let me know if you have a better way to write it or if you have another opinion!
There was a problem hiding this comment.
I agree that we should print all messages after the stack name output, so can we move all of those messages after the stack name output and move the stack name to printSecurityDiff?
There was a problem hiding this comment.
If not, can you explain which messages are unable to be moved?
There was a problem hiding this comment.
Thanks @comcalvi.
I think such an implementation is possible.
I have one question:
I forgot to mention that in the current code, the messages generated during the changeset creation are displayed before the stack name.
Is this acceptable?
To fix this issue, it is necessary to control the display of the stack name based on the presence of a diff when the --quiet flag is true.
However, determining the diff requires creating a changeset (though there are cases where it is unnecessary), and messages are output during this process.
While it might be necessary to suppress these messages when the --quiet flag is true, debug messages are also output during the changeset creation process when the -v flag is specified.
We would need to allow cases where messages output during the changeset creation process appear before the stack name.
There was a problem hiding this comment.
For example, I am thinking of changing the printStackDiff function to look like this
export function printStackDiff(
oldTemplate: any,
newTemplate: cxapi.CloudFormationStackArtifact,
strict: boolean,
context: number,
quiet: boolean,
+ stackName?: string,
changeSet?: DescribeChangeSetOutput,
isImport?: boolean,
stream: FormatStream = process.stderr,
nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }): number {
let diff = fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);
+ // must output the stack name if there are differences, even if quiet
+ if (stackName && (!quiet || !diff.isEmpty)) {
+ stream.write(format('Stack %s\n', chalk.bold(stackName)));
+ }
+ if (!quiet && isImport) {
+ stream.write('Parameters and rules created during migration do not affect resource configuration.\n');
+ }
// detect and filter out mangled characters from the diff
let filteredChangesCount = 0;
if (diff.differenceCount && !strict) {
const mangledNewTemplate = JSON.parse(mangleLikeCloudFormation(JSON.stringify(newTemplate.template)));
const mangledDiff = fullDiff(oldTemplate, mangledNewTemplate, changeSet);
filteredChangesCount = Math.max(0, diff.differenceCount - mangledDiff.differenceCount);
if (filteredChangesCount > 0) {
diff = mangledDiff;
}
}
// filter out 'AWS::CDK::Metadata' resources from the template
if (diff.resources && !strict) {
diff.resources = diff.resources.filter(change => {
if (!change) { return true; }
if (change.newResourceType === 'AWS::CDK::Metadata') { return false; }
if (change.oldResourceType === 'AWS::CDK::Metadata') { return false; }
return true;
});
}
let stackDiffCount = 0;
if (!diff.isEmpty) {
stackDiffCount++;
formatDifferences(stream, diff, {
...logicalIdMapFromTemplate(oldTemplate),
...buildLogicalToPathMap(newTemplate),
}, context);
} else if (!quiet) {
print(chalk.green('There were no differences'));
}
if (filteredChangesCount > 0) {
print(chalk.yellow(`Omitted ${filteredChangesCount} changes because they are likely mangled non-ASCII characters. Use --strict to print them.`));
}
for (const nestedStackLogicalId of Object.keys(nestedStackTemplates ?? {})) {
if (!nestedStackTemplates) {
break;
}
const nestedStack = nestedStackTemplates[nestedStackLogicalId];
- if (!quiet) {
- stream.write(format('Stack %s\n', chalk.bold(nestedStack.physicalName ?? nestedStackLogicalId)));
- }
(newTemplate as any)._template = nestedStack.generatedTemplate;
stackDiffCount += printStackDiff(
nestedStack.deployedTemplate,
newTemplate,
strict,
context,
quiet,
+ nestedStack.physicalName ?? nestedStackLogicalId,
undefined,
isImport,
stream,
nestedStack.nestedStackTemplates,
);
}
return stackDiffCount;
}There was a problem hiding this comment.
I see no harm in creating the changeset before printing the stack name. Since we now need the changeset to determine the diff, I think that is what we must do.
cdk diff --quiet to print stack name when there is diffscdk diff with --quiet option to print stack name when there is diffs
cdk diff with --quiet option to print stack name when there is diffscdk diff --quiet to print stack name when there is diffs
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
…x-quiet-stack-name
…/aws-cdk into fix-quiet-stack-name
|
@comcalvi |
|
Is there any update on this PR? |
|
How can we grab the attention of someone who can review this? |
comcalvi
left a comment
There was a problem hiding this comment.
This is good to merge, pending the success of the CLI test run.
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #27128
Reason for this change
The
--quietflag on thecdk diffcommand prevents the stack name and default message from being printed when no diff exists.If diffs exist, the stack name and diffs are expected to be printed, but currently, the stack name is not printed, and it is difficult to determine which stack the diff is for.
for example:
$ cdk diff --quiet Resources [~] AWS::S3::Bucket MyFirstBucket MyFirstBucketB8884501 ├─ [~] DeletionPolicy │ ├─ [-] Delete │ └─ [+] Retain └─ [~] UpdateReplacePolicy ├─ [-] Delete └─ [+] Retain ✨ Number of stacks with differences: 1This PR will fix to print the stack name when the
--quietflag is specified and diffs exist.Description of changes
Changed the position of the
fullDifffunction call.It is possible to output the stack name in the
printSecurityDifforprintStackDifffunctions, but since the message has already been output before these functions are called, the stack name must be output first.I think it would be more user-friendly to have all messages after the output of the stack name, but if this is not the case, please point this out.
Description of how you validated changes
I added a unit test to confirm to print the stack name when diff exists.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license