Skip to content
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.

Sign up for GitHub

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(CLI): change-set diff not required for new stack diffs #29268

Closed
wants to merge 14 commits into from
Closed

fix(CLI): change-set diff not required for new stack diffs #29268

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bf7c7bb
add check for new stak in fullDiff function
sumupitchayan Feb 26, 2024
ec2fe4e
add unit test
sumupitchayan Feb 26, 2024
f578824
fix diff test expect
sumupitchayan Feb 27, 2024
f793b87
expect NewResource change impact to be WILL_CREATE in test
sumupitchayan Feb 27, 2024
cedf900
Merge branch 'main' into sumughan/fix-changeset-delete-stack
sumupitchayan Feb 27, 2024
400147e
fix diff test string problems in test expect
sumupitchayan Feb 27, 2024
4df1cf0
Merge main
sumupitchayan Feb 27, 2024
0d2fc04
Merge branch 'main' into sumughan/fix-changeset-delete-stack
sumupitchayan Feb 27, 2024
bcb544c
try fixing migrate import diff in cdktoolkit
sumupitchayan Feb 28, 2024
01071bd
Revert changes to import test expect
sumupitchayan Feb 28, 2024
d4cb526
Merge branch 'sumughan/fix-changeset-delete-stack' of https://github.…
sumupitchayan Feb 28, 2024
7b678ca
add important info for all changeSets, just don't do filterFalsePosit…
sumupitchayan Feb 28, 2024
bb73010
Revert "try fixing migrate import diff in cdktoolkit"
sumupitchayan Feb 28, 2024
c94c8ae
Merge branch 'main' into sumughan/fix-changeset-delete-stack
sumupitchayan Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export function fullDiff(
normalize(currentTemplate);
normalize(newTemplate);
const theDiff = diffTemplate(currentTemplate, newTemplate);
if (changeSet) {
// If the currentTemplate is empty, we skip these changeset functions.
if (changeSet && Object.keys(currentTemplate).length > 0) {
filterFalsePositivies(theDiff, changeSet);
addImportInformation(theDiff, changeSet);
}
Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,4 +1231,22 @@ describe('changeset', () => {
expect(differences.resources.differenceCount).toBe(1);
expect(differences.resources.get('BucketResource').changeImpact === ResourceImpact.WILL_IMPORT);
});

test('does not do changeset checks for new stack', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something but I'm not sure how this tests that we do not do a changeset for new stacks. Could you clarify that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was confused on this too - my logic was that calling fullDiff with the current stack being empty (new) should mean that there should be only 1 difference count, and the change impact should be ResourceImpact.WILL_CREATE; I'm not sure how else to test that we don't do changeset for new stacks

// GIVEN
const currentTemplate = {};

// WHEN
const newTemplate = {
Resources: {
NewResource: {
Type: 'AWS::Something::Resource',
},
},
};

let differences = fullDiff(currentTemplate, newTemplate);
expect(differences.resources.differenceCount).toBe(1);
expect(differences.resources.get('NewResource').changeImpact).toBe(ResourceImpact.WILL_CREATE);
});
});
14 changes: 13 additions & 1 deletion packages/aws-cdk/lib/cdk-toolkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { Concurrency, WorkGraph } from './util/work-graph';
import { WorkGraphBuilder } from './util/work-graph-builder';
import { AssetBuildNode, AssetPublishNode, StackNode } from './util/work-graph-types';
import { environmentsFromDescriptors, globEnvironmentsFromStacks, looksLikeGlob } from '../lib/api/cxapp/environments';
import { CloudFormation } from 'aws-sdk';

export interface CdkToolkitProps {

Expand Down Expand Up @@ -167,7 +168,12 @@ export class CdkToolkit {
removeNonImportResources(stack);
}

const changeSet = options.changeSet ? await createDiffChangeSet({
let resourcesToImportLogicalIds = new Set<string>();
resourcesToImport?.forEach((value: CloudFormation.ResourceToImport, _index: number, _array: CloudFormation.ResourceToImport[]) => {
resourcesToImportLogicalIds.add(value.LogicalResourceId);
});

let changeSet = options.changeSet ? await createDiffChangeSet({
stack,
uuid: uuid.v4(),
deployments: this.props.deployments,
Expand All @@ -178,6 +184,12 @@ export class CdkToolkit {
stream,
}) : undefined;

changeSet?.Changes?.forEach((value: CloudFormation.Change, _index: number, _array: CloudFormation.Change[]) => {
if (resourcesToImportLogicalIds.has(value.ResourceChange?.LogicalResourceId!)) {
value.ResourceChange!.Action = 'Import';
}
});

if (resourcesToImport) {
stream.write('Parameters and rules created during migration do not affect resource configuration.\n');
}
Expand Down
Loading