-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cdk): cdk diff --quiet
to print stack name when there is diffs
#30186
base: main
Are you sure you want to change the base?
Conversation
@@ -393,15 +393,37 @@ describe('non-nested stacks', () => { | |||
|
|||
// WHEN | |||
const exitCode = await toolkit.diff({ | |||
stackNames: ['A', 'A'], | |||
stackNames: ['D'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (stackArtifact.stackName === 'D') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@@ -191,14 +188,21 @@ export class CdkToolkit { | |||
} | |||
} | |||
|
|||
const diff = options.securityOnly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, can you explain which messages are unable to be moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
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
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -191,14 +188,21 @@ export class CdkToolkit { | |||
} | |||
} | |||
|
|||
const diff = options.securityOnly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
? fullDiff(currentTemplate, stack.template, changeSet) | ||
: fullDiff(currentTemplate, stack.template, changeSet, !!resourcesToImport); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be calling fullDiff
across both this method and printSecurityDiff
. I'd prefer we keep fullDiff
in printSecurityDiff
, and then move the stack print to printSecurityDiff
.
@@ -191,14 +188,21 @@ export class CdkToolkit { | |||
} | |||
} | |||
|
|||
const diff = options.securityOnly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, can you explain which messages are unable to be moved?
Issue # (if applicable)
Closes #27128
Reason for this change
The
--quiet
flag on thecdk diff
command 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:
This PR will fix to print the stack name when the
--quiet
flag is specified and diffs exist.Description of changes
Changed the position of the
fullDiff
function call.It is possible to output the stack name in the
printSecurityDiff
orprintStackDiff
functions, 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