Skip to content

feat(toolkit-lib): fallBackToTemplate option for diffs #354

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

Merged
merged 3 commits into from
Apr 14, 2025
Merged

feat(toolkit-lib): fallBackToTemplate option for diffs #354

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2791490
feat(toolkit-lib): fallBackToTemplate option for diffs
kaizencc Apr 10, 2025
e3d542f
Merge branch 'main' into conroy/fallBackToTemplate
kaizencc Apr 11, 2025
201c7a3
rename to be more generic
kaizencc Apr 11, 2025
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
22 changes: 17 additions & 5 deletions packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,14 @@ export type PrepareChangeSetOptions = {
sdkProvider: SdkProvider;
parameters: { [name: string]: string | undefined };
resourcesToImport?: ResourcesToImport;
/**
* Enable falling back to template-based diff in case creating the changeset is not possible or results in an error.
*
* Should be used for stacks containing nested stacks or when change set permissions aren't available.
*
* @default true
*/
fallBackToTemplate?: boolean;
Copy link
Contributor

@rix0rrr rix0rrr Apr 11, 2025

Choose a reason for hiding this comment

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

This doesn't seem to be added on a type that is only used for the diff operation.

For example, resourcesToImport definitely should not be part of the diff operation. Can't we place fallBackToTemplate elsewhere?

And if it is the case that uploadBodyParameterAndCreateChangeSet is also used in cdk import, then the error message "we will fall back to a template diff" doesn't make any sense.

Can you categorize the uses of this function a bit better?

Copy link
Contributor Author

@kaizencc kaizencc Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

right now, createDiffChangeSet is the only one that uses PrepareChangeSetOptions. uploadBodyParameterAndCreateChangeSet is a helper method of createDiffChangeSet and not exposed or used elsewhere. and in turn, createDiffChangeSet is only used in the diff operation -- i believe it's meant to be diff specific. The error message / IOHost messages have existed previously, and are diff specific. are you asking me to refactor uploadBodyParameterAndCreateChangeSet to be more generic so that it may be used in cdk import in the future? i'd rather not in this PR.

would it be better if I renamed fallBackToTemplate as a more generic failOnError?

}

export type CreateChangeSetOptions = {
Expand Down Expand Up @@ -240,12 +248,16 @@ async function uploadBodyParameterAndCreateChangeSet(
role: executionRoleArn,
});
} catch (e: any) {
await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(String(e)));
await ioHelper.notify(IO.DEFAULT_TOOLKIT_INFO.msg(
'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n',
));
if (options.fallBackToTemplate ?? true) {
await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(String(e)));
await ioHelper.notify(IO.DEFAULT_TOOLKIT_INFO.msg(
'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n',
));

return undefined;
return undefined;
}

throw new ToolkitError('Could not create a change set, set fallBackToTemplate=true to base the diff on template differences\n', e);
}
}

Expand Down
17 changes: 8 additions & 9 deletions packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@ export interface CloudFormationDiffOptions {
}

export interface ChangeSetDiffOptions extends CloudFormationDiffOptions {
// @TODO: add this as a feature
// /**
// * Enable falling back to template-based diff in case creating the changeset is not possible or results in an error.
// *
// * Should be used for stacks containing nested stacks or when change set permissions aren't available.
// *
// * @default true
// */
// readonly fallbackToTemplate?: boolean;
/**
* Enable falling back to template-based diff in case creating the changeset is not possible or results in an error.
*
* Should be used for stacks containing nested stacks or when change set permissions aren't available.
*
* @default true
*/
readonly fallbackToTemplate?: boolean;

/**
* Additional parameters for CloudFormation when creating a diff change set
Expand Down
26 changes: 23 additions & 3 deletions packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ async function cfnDiff(
deployments: Deployments,
options: DiffOptions,
sdkProvider: SdkProvider,
changeSet: boolean,
includeChangeSet: boolean,
): Promise<TemplateInfo[]> {
const templateInfos = [];
const methodOptions = (options.method?.options ?? {}) as ChangeSetDiffOptions;
Expand All @@ -78,13 +78,23 @@ async function cfnDiff(
removeNonImportResources(stack);
}

const changeSet = includeChangeSet ? await changeSetDiff(
ioHelper,
deployments,
stack,
sdkProvider,
resourcesToImport,
methodOptions.parameters,
methodOptions.fallbackToTemplate,
) : undefined;

templateInfos.push({
oldTemplate: currentTemplate,
newTemplate: stack,
stackName: stack.stackName,
isImport: !!resourcesToImport,
nestedStacks,
changeSet: changeSet ? await changeSetDiff(ioHelper, deployments, stack, sdkProvider, resourcesToImport, methodOptions.parameters) : undefined,
changeSet,
});
}

Expand All @@ -98,6 +108,7 @@ async function changeSetDiff(
sdkProvider: SdkProvider,
resourcesToImport?: ResourcesToImport,
parameters: { [name: string]: string | undefined } = {},
fallBackToTemplate: boolean = true,
): Promise<any | undefined> {
let stackExists = false;
try {
Expand All @@ -107,6 +118,10 @@ async function changeSetDiff(
tryLookupRole: true,
});
} catch (e: any) {
if (!fallBackToTemplate) {
throw new ToolkitError(`describeStacks call failed with ${e} for ${stack.stackName}, set fallBackToTemplate to true or use DiffMethod.templateOnly to base the diff on template differences.`);
}

await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(`Checking if the stack ${stack.stackName} exists before creating the changeset has failed, will base the diff on template differences.\n`));
await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(formatErrorMessage(e)));
stackExists = false;
Expand All @@ -121,9 +136,14 @@ async function changeSetDiff(
sdkProvider,
parameters: parameters,
resourcesToImport,
fallBackToTemplate,
});
} else {
await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(`the stack '${stack.stackName}' has not been deployed to CloudFormation or describeStacks call failed, skipping changeset creation.`));
if (!fallBackToTemplate) {
throw new ToolkitError(`the stack '${stack.stackName}' has not been deployed to CloudFormation, set fallBackToTemplate to true or use DiffMethod.templateOnly to base the diff on template differences.`);
}

await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(`the stack '${stack.stackName}' has not been deployed to CloudFormation, skipping changeset creation.`));
return;
}
}
Expand Down
46 changes: 42 additions & 4 deletions packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import * as path from 'path';
import { format } from 'util';
import * as chalk from 'chalk';
import { DiffMethod } from '../../lib/actions/diff';
import * as apis from '../../lib/api/shared-private';
import { RequireApproval } from '../../lib/api/shared-private';
import { StackSelectionStrategy } from '../../lib/api/shared-public';
import { Toolkit } from '../../lib/toolkit';
import { builderFixture, TestIoHost } from '../_helpers';
import { MockSdk } from '../_helpers/mock-sdk';

let ioHost: TestIoHost;
let toolkit: Toolkit;
Expand All @@ -18,7 +16,6 @@ beforeEach(() => {
ioHost.requireDeployApproval = RequireApproval.NEVER;

toolkit = new Toolkit({ ioHost });
const sdk = new MockSdk();

// Some default implementations
jest.spyOn(apis.Deployments.prototype, 'readCurrentTemplateWithNestedStacks').mockResolvedValue({
Expand Down Expand Up @@ -177,7 +174,48 @@ describe('diff', () => {
}));
});

describe('templatePath', () => {
describe('DiffMethod.ChangeSet', () => {
test('ChangeSet diff method falls back to template only if changeset not found', async () => {
// WHEN
ioHost.level = 'debug';
const cx = await builderFixture(toolkit, 'stack-with-bucket');
await toolkit.diff(cx, {
stacks: { strategy: StackSelectionStrategy.ALL_STACKS },
method: DiffMethod.ChangeSet(),
});

// THEN
expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
action: 'diff',
level: 'info',
code: 'CDK_TOOLKIT_I0000',
message: expect.stringContaining('Could not create a change set, will base the diff on template differences'),
}));
});

test('ChangeSet diff method throws if changeSet fails and fallBackToTemplate = false', async () => {
// WHEN
const cx = await builderFixture(toolkit, 'stack-with-bucket');
await expect(async () => toolkit.diff(cx, {
stacks: { strategy: StackSelectionStrategy.ALL_STACKS },
method: DiffMethod.ChangeSet({ fallbackToTemplate: false }),
})).rejects.toThrow(/Could not create a change set, set fallBackToTemplate=true/);
});

test('ChangeSet diff method throws if stack not found and fallBackToTemplate = false', async () => {
// GIVEN
jest.spyOn(apis.Deployments.prototype, 'stackExists').mockResolvedValue(false);

// WHEN
const cx = await builderFixture(toolkit, 'stack-with-bucket');
await expect(async () => toolkit.diff(cx, {
stacks: { strategy: StackSelectionStrategy.ALL_STACKS },
method: DiffMethod.ChangeSet({ fallbackToTemplate: false }),
})).rejects.toThrow(/the stack 'Stack1' has not been deployed to CloudFormation/);
});
});

describe('DiffMethod.LocalFile', () => {
test('fails with multiple stacks', async () => {
// WHEN + THEN
const cx = await builderFixture(toolkit, 'two-empty-stacks');
Expand Down
Loading