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.

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(core): Add stage prefix to stack name shortening process #24443

Merged
merged 23 commits into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8abb431
Add prefix to stack name shortening process
QustodioPablo Mar 3, 2023
668539c
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
QustodioPablo Mar 3, 2023
6c1532b
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
QustodioPablo Mar 6, 2023
7f06cd7
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
QustodioPablo Mar 21, 2023
b28f46f
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
QustodioPablo Mar 22, 2023
0cf310a
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Mar 28, 2023
b126e20
Merge branch 'QustodioPablo/fix-stack-name-too-long' of github.com:Qu…
QustodioPablo Mar 28, 2023
1f5b03b
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Mar 29, 2023
b151d21
Process prefix inside the makeUniqueResourceName function
QustodioPablo Mar 29, 2023
b1aee08
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Mar 30, 2023
ecfc9cb
Fixed scenario of Stage with only one default component
QustodioPablo Mar 30, 2023
297273a
Fixed missing brace
QustodioPablo Mar 30, 2023
16e96c1
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Mar 30, 2023
0e28b23
Use variable
QustodioPablo Mar 31, 2023
f52804e
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Mar 31, 2023
adecdbc
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Apr 3, 2023
f979e85
Recovered changes
QustodioPablo Apr 3, 2023
e9cd174
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
QustodioPablo Apr 4, 2023
c6c6053
Added prefix as option
QustodioPablo Apr 5, 2023
7655cd2
Merge branch 'QustodioPablo/fix-stack-name-too-long' of github.com:Qu…
QustodioPablo Apr 5, 2023
56bd828
Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…
QustodioPablo Apr 5, 2023
5dd29bc
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
QustodioPablo Apr 11, 2023
729f23e
Merge branch 'main' into QustodioPablo/fix-stack-name-too-long
mergify[bot] Apr 11, 2023
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
10 changes: 7 additions & 3 deletions packages/@aws-cdk/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1417,7 +1417,7 @@ export class Stack extends Construct implements ITaggable {
private generateStackName() {
const assembly = Stage.of(this);
const prefix = (assembly && assembly.stageName) ? `${assembly.stageName}-` : '';
return `${prefix}${this.generateStackId(assembly)}`;
return `${this.generateStackId(assembly, prefix)}`;
}

/**
Expand All @@ -1432,7 +1432,7 @@ export class Stack extends Construct implements ITaggable {
/**
* Generate an ID with respect to the given container construct.
*/
private generateStackId(container: IConstruct | undefined) {
private generateStackId(container: IConstruct | undefined, prefix: string='') {
const rootPath = rootPathTo(this, container);
const ids = rootPath.map(c => Node.of(c).id);

Expand All @@ -1442,6 +1442,10 @@ export class Stack extends Construct implements ITaggable {
throw new Error('unexpected: stack id must always be defined');
}

if (prefix != '') {
ids[0] = `${prefix}${ids[0]}`;
}

return makeStackName(ids);
}

Expand Down Expand Up @@ -1629,7 +1633,7 @@ export function rootPathTo(construct: IConstruct, ancestor?: IConstruct): IConst
* behavior.
*/
function makeStackName(components: string[]) {
if (components.length === 1) { return components[0]; }
if (components.length === 1 && components[0].length <= 128) { return components[0]; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Iirc, this check is done inside makeUniqueResourceName so it shouldn't be needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check done inside makeUniqueResourceName will remove the - from the prefix, as it removes anything that is not a number or a character, unless you specify to keep a special character though parameters, but I was afraid of breaking compatibility with already existing stacks if doing so.
I'll make changes to pass the prefix to the makeUniqueResourceName as an optional parameter as you mentioned in another comment.
Thanks!

return makeUniqueResourceName(components, { maxLength: 128 });
}

Expand Down
11 changes: 11 additions & 0 deletions packages/@aws-cdk/core/test/stage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@ describe('stage', () => {
expect(stack.stackName).toEqual('MyStage-MyStack');
});

test('generated stack names will not exceed 128 characters', () => {
// WHEN
const app = new App();
const stage = new Stage(app, 'ThisStageNameIsVeryLongButWillOnlyBeTooLongWhenCombinedWithTheStackName');
const stack = new BogusStack(stage, 'ThisStackNameIsVeryLongButItWillOnlyBeTooLongWhenCombinedWithTheLongPrefix');

// THEN
expect(stack.stackName.length).toEqual(128);
expect(stack.stackName).toEqual('ThisStageNameIsVeryLongButWillOnlyBeTooLongWhenCombinedWithTsVeryLongButItWillOnlyBeTooLongWhenCombinedWithTheLongPrefixA24103F6');
});

test('Can not have dependencies to stacks outside the nested asm', () => {
// GIVEN
const app = new App();
Expand Down