-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): use node.path in skip bundling check for consistency with cdk deploy CLI #19950
fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI #19950
Conversation
Not sure if this PR handles this case, but another issue, in addition to #12898 (fixed) and #19927 (open) is related to this issue: #16830 Because of #16830, I need to prefix the stack name with the pipeline ID (otherwise it will try to destroy stuff). Example:
This then tries to bundle the entire root dir (as described in my comment here: #12898 (comment)). |
@moltar the part about logical ID name changes when in a pipeline or not isn't related to this. The part where deployment with |
Right, did not mean to imply that it was, sorry. 😁 Meant that it was "softly" related in a sense, that pipeline prefix bug causes another corner case, related to this issue.
That's what I meant. From my investigation before, it was about the naming. I posted my findings in that same issue. Basically, it was the difference between |
Yep, if bundling is skipped for a stack when it was selected for deploy in the CLI, a downstream affect that should probably be addressed is it uploads the entire root directory of the project. It would be good to track down where this happens and fail with an "asset not found" message. The change in this PR should prevent it from happening by matching against the same value ( The inconsistency exists because the value used for pattern matching in the CLI (on
This mismatch is what causes bundling to be skipped and consequently the project dir to be uploaded instead. |
Yes, definitely a good idea. I think uploading the entire dir is not only annoying but could be a security concern. |
any progress with this? |
Waiting for CDK team to review/merge... |
@stevehodgkiss this will need to target the |
The pattern given to aws-cdk on the CLI is matched against `hierarchicalId` (i.e. displayName, node.path) [1] [2] [3]. The same pattern is given to the CDK app but was matching against `stackName`, causing potential for bundling to be skipped under certain scenarios (i.e. using --exclusively with custom stack names). To make them consistent, the skip bundling check has been changed to use the same value (node.path/hierarchicalId/displayName) as aws-cdk when deciding whether to bundle an asset. fixes aws#19927 [1] https://github.com/aws/aws-cdk/blob/1d0270446b3effa6b8518de3c7d76f0c14e626c5/packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts#L138 [2] https://github.com/aws/aws-cdk/blob/6cca62db88866479f2b1d4f52b30beab3d78aeb2/packages/@aws-cdk/cx-api/lib/cloud-artifact.ts#L143-L145 [3] https://github.com/aws/aws-cdk/blob/6cca62db88866479f2b1d4f52b30beab3d78aeb2/packages/@aws-cdk/core/lib/stack-synthesizers/_shared.ts#L66
c00ff00
to
60e5bbd
Compare
@kaizencc yep, I've rebased and updated the merge target to |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…-logic-as-aws-cdk
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
… for consistency with cdk deploy CLI" (#21174) #19950 breaks users who use bundling inside a pipeline stack. Previously we converted `Stage/Stack` to `Stage-Stack` and left a comment as to why that was necessary. This comment was both insufficient and disregarded, which resulted in different behavior between the bundling logic and skip bundling logic. The asset path for skip bundling would not longer be the same as the asset path when bundling, so the asset would not be found.
… for consistency with cdk deploy CLI" (#21174) #19950 breaks users who use bundling inside a pipeline stack. Previously we converted `Stage/Stack` to `Stage-Stack` and left a comment as to why that was necessary. This comment was both insufficient and disregarded, which resulted in different behavior between the bundling logic and skip bundling logic. The asset path for skip bundling would not longer be the same as the asset path when bundling, so the asset would not be found.
… for consistency with cdk deploy CLI" (aws#21174) aws#19950 breaks users who use bundling inside a pipeline stack. Previously we converted `Stage/Stack` to `Stage-Stack` and left a comment as to why that was necessary. This comment was both insufficient and disregarded, which resulted in different behavior between the bundling logic and skip bundling logic. The asset path for skip bundling would not longer be the same as the asset path when bundling, so the asset would not be found.
…m stack name (#21248) Fixes #19927 [A previous PR](#19950) attempted this fix, but it caused another issue where the default pattern (`['*']`, used on cdk synth/deploy when a pattern isn't supplied) doesn't match stacks under a stage (`stage/stack`), and that caused bundling to be skipped. The default pattern worked (matched all stacks) previously because stackName was being used (and there are no `/` in stack names). See [this comment](#19927 (comment)) in the issue for more details on this. The first commit 5556b60 adds special handling to the default pattern so that it always returns true (bypassed minimatch which would return false if a stack is under a stage). The second commit a9a48a0 aims to remove the duplication of this pattern across the codebase, by defaulting `BUNDLING_STACKS` to `undefined` and returning true in bundlingRequired if that's the case. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The pattern given to aws-cdk on the CLI (
cdk deploy $pattern
) is matched againststack.hierarchicalId
(i.e. displayName, node.path) [1] [2] [3].The same pattern is given to the CDK app (as
bundlingStacks
) but is matching againststackName
, causing potential for bundling to be skipped under certain scenarios (i.e. using --exclusively with custom stack names).To make them consistent, the skip bundling check has been changed to match against the same value as
cdk deploy $pattern
(node.path/hierarchicalId/displayName) when deciding whether to bundle an asset. Making them consistent ensures necessary stacks are bundled, avoiding the issue of uploading the entire project directory since the asset wasn't bundled.fixes #19927
[1]
aws-cdk/packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts
Line 138 in 1d02704
[2]
aws-cdk/packages/@aws-cdk/cx-api/lib/cloud-artifact.ts
Lines 143 to 145 in 6cca62d
[3]
aws-cdk/packages/@aws-cdk/core/lib/stack-synthesizers/_shared.ts
Line 66 in 6cca62d
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license