Skip to content

Conversation

plumdog
Copy link
Contributor

@plumdog plumdog commented Sep 8, 2022

Fixes #21925.

cxapi.BUNDLING_STACKS refers to stacks by their logical path within the app, so use this.node.path when comparing.

Previous behaviour only worked because:

  • For "top level" stacks, the default name == id == path
  • For stacks nested one layer deep in a stage, default name == ${stage}-${id} == path.replace('/', '-')

I've added tests that fail when:

  • The stack has a custom name, not the id
  • The stack is not nested one layer deep in a stage, so .replace('/', '-') doesn't fudge the path into the name

Question: while I have verified the above regarding cxapi.BUNDLING_STACKS experimentally, I haven't found anything that documents this.


All Submissions:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Sep 8, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team September 8, 2022 08:19
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Sep 8, 2022
@plumdog plumdog force-pushed the issue-21925-fix-asset-skipping branch from 4552b57 to ae88cc5 Compare September 8, 2022 08:20
this.stackName,
pattern.replace('/', '-'),
));
return bundlingStacks.some(pattern => minimatch(this.node.path, pattern));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to add a link here to something that documents cxapi.BUNDLING_STACKS.

@plumdog plumdog force-pushed the issue-21925-fix-asset-skipping branch from ae88cc5 to 3200d98 Compare September 8, 2022 08:24
@mrgrain
Copy link
Contributor

mrgrain commented Sep 8, 2022

Hey @plumdog thanks for looking into this! Unfortunately your PR is basically a revert of this revert: #21174
Whatever we do, we need to make sure we don't break bundling inside a pipeline stack again.

PS: I have little context on the issue, just remembered the revert. cc @kaizencc @corymhall

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7aac474
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@plumdog
Copy link
Contributor Author

plumdog commented Sep 8, 2022

@mrgrain aha.

My remaining question is, I think "what is cxapi.BUNDLING_STACKS and - crucially - how can a stack tell from that whether it should bundle?"

I know it was a revert, but I'd be curious to see the broken behaviour that #21174 fixed - in other words, what would a failing for that PR look like?

@mrgrain
Copy link
Contributor

mrgrain commented Sep 8, 2022

@plumdog The issue surfaced as the error described in #18459 when bundling in a Pipeline stage (But not necessarily the same scenarios the issue describes. However the issue feels also very relevant tbh).

There is also an other active PR attempting to fix this: #21248

@plumdog
Copy link
Contributor Author

plumdog commented Sep 8, 2022

@mrgrain Gotcha! So yeah, my findings match with those of #21248.

Thank you for your help, but I'm happy to close this PR and wait for #21248.

@plumdog plumdog closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core: incorrect asset bundling skip for assets in stacks inside stages inside stacks
3 participants