fix(core): asset bundling skipped when using --exclusively with custom stack name #21248

stevehodgkiss · 2022-07-20T10:34:27Z

Fixes #19927

A previous PR 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 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?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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 original fix as per 5cff2d9, with the addition of bypassing minimatch when `*` is the given pattern (return true). This gives special meaning to `*`, since `*` in minimatch doesn't produce the desired selection (all stacks). fixes aws#19927

gitpod-io · 2022-07-20T10:34:34Z

stevehodgkiss · 2022-07-20T10:43:04Z

packages/aws-cdk/lib/api/cxapp/exec.ts

+  const bundlingStacks = config.settings.get(['bundlingStacks']);
+  if (bundlingStacks) {
+    context[cxapi.BUNDLING_STACKS] = bundlingStacks;
+  }


An alternative to allowing bundlingStacks to be undefined when no pattern is supplied on the CLI (as in these changes) would be to set a valid "all" pattern for minimatch here (i.e. globstar ** which matches stacks under a stage) and in settings.ts where the CLI input is parsed. Or handling ['*'] as a special case outside of minimatch (which the first commit does).

I think I would be fine updating it to use ** since I would imagine that was the intention from the start.

I agree, I'd rather have fewer ifs (fewer branches ⇒ fewer things that can behave unexpectedly).

So defaulting to ** seems correct.

Updated. It's worth noting that users explicitly specifying * on cdk deploy '*' will need to update it to ** in order to match stacks under stages. Similarly, patterns like *StackNameEnd will need updating to **/*StackNameEnd.

I actually think this is fine. Currently specifying * or *StackNameEnd does not work for stacks under a stage, you have to do */* or */*StackNameEnd which still work after this change.

TheRealAmazonKendra · 2022-07-28T23:47:23Z

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

rix0rrr

See discussion

Pull request has been modified.

plumdog · 2022-09-08T11:32:42Z

I opened #21925 and chased it for a while before @mrgrain helpfully pointed me at this PR.

My issue is where there are multiple layers of nesting, but I think it too will be fixed by the change in this PR. See #21925 (comment)

Edit: only other thing I might be able to add is: do you want this test here: https://github.com/aws/aws-cdk/pull/21962/files#diff-f408c19e5bd715b329b76439aaf92dfe47c956a0277896b04e9a0c52b7cbcfe2R939-R961 ?

…-use-same-logic-as-aws-cdk-take-2

plumdog · 2022-09-09T15:31:32Z

Compromise thought: bundle if either the new (path) or old (name) logic says to bundle.

This is messy, but guarantees backwards compatibility (at least, won't stop bundling something that was bundled before). Also, I don't really understand why the old name-based logic was required to make pipeline deployments work. I'm not sure if there is a unit test for it anywhere, so I'm not sure if this change, as it stands, will cause that to regress.

…cdk-take-2

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

corymhall · 2022-10-10T15:49:32Z

Added example-integ-test because I added an integ test for this separately via #21294

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

corymhall

@stevehodgkiss sorry for the delay on this. I wanted to make sure I tested this out and I finally was able to.

mergify · 2022-10-10T15:51:28Z

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).

…cdk-take-2

aws-cdk-automation · 2022-10-10T16:29:40Z

AWS CodeBuild CI Report

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

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

mergify · 2022-10-10T16:29:57Z

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-cdk-automation requested a review from a team July 20, 2022 10:34

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jul 20, 2022

stevehodgkiss commented Jul 20, 2022

View reviewed changes

stevehodgkiss mentioned this pull request Jul 20, 2022

(aws-cdk): asset bundling skipped when using --exclusively with custom stack name #19927

Closed

corymhall self-assigned this Aug 3, 2022

rix0rrr previously requested changes Aug 8, 2022

View reviewed changes

stevehodgkiss added 2 commits August 22, 2022 10:07

default to ** when a value isn't provided

9f5c66d

Update readme to use '**' for cdk deploy

dc97adc

stevehodgkiss force-pushed the fix-bundling-check-to-use-same-logic-as-aws-cdk-take-2 branch from a9a48a0 to dc97adc Compare August 22, 2022 03:07

stevehodgkiss changed the title ~~fix(core): make bundlingRequired minimatch matching the same as in aws-cdk CLI - take 2~~ fix(core): asset bundling skipped when using --exclusively with custom stack name Aug 22, 2022

stevehodgkiss marked this pull request as ready for review August 22, 2022 04:16

mrgrain mentioned this pull request Sep 8, 2022

fix(core): correctly skip asset bundling #21962

Closed

1 task

mrgrain requested review from rix0rrr, corymhall and TheRealAmazonKendra September 8, 2022 11:42

stevehodgkiss added 2 commits September 9, 2022 08:25

Add test for Stack/Stage/Stack bundling behaviour

f9e633d

Merge remote-tracking branch 'origin/main' into fix-bundling-check-to…

d1dba12

…-use-same-logic-as-aws-cdk-take-2

stevehodgkiss force-pushed the fix-bundling-check-to-use-same-logic-as-aws-cdk-take-2 branch from 306cc26 to d1dba12 Compare September 9, 2022 01:32

Merge branch 'main' into fix-bundling-check-to-use-same-logic-as-aws-…

e46d505

…cdk-take-2

aws-cdk-automation previously requested changes Oct 7, 2022

View reviewed changes

corymhall added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Oct 10, 2022

corymhall approved these changes Oct 10, 2022

View reviewed changes

Merge branch 'main' into fix-bundling-check-to-use-same-logic-as-aws-…

b699dfa

…cdk-take-2

mergify bot merged commit 209ddea into aws:main Oct 10, 2022

corymhall mentioned this pull request Nov 10, 2022

CDK deploy: using exclusively tag creates incorrect lambda artifacts in nested stack #22834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): asset bundling skipped when using --exclusively with custom stack name #21248

fix(core): asset bundling skipped when using --exclusively with custom stack name #21248

stevehodgkiss commented Jul 20, 2022

gitpod-io bot commented Jul 20, 2022

stevehodgkiss Jul 20, 2022

corymhall Aug 3, 2022

rix0rrr Aug 8, 2022

stevehodgkiss Aug 22, 2022

corymhall Oct 10, 2022

TheRealAmazonKendra commented Jul 28, 2022

rix0rrr left a comment

plumdog commented Sep 8, 2022 •

edited

plumdog commented Sep 9, 2022

aws-cdk-automation left a comment

corymhall commented Oct 10, 2022

corymhall left a comment

mergify bot commented Oct 10, 2022

aws-cdk-automation commented Oct 10, 2022

mergify bot commented Oct 10, 2022

fix(core): asset bundling skipped when using --exclusively with custom stack name #21248

fix(core): asset bundling skipped when using --exclusively with custom stack name #21248

Conversation

stevehodgkiss commented Jul 20, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 20, 2022

stevehodgkiss Jul 20, 2022

Choose a reason for hiding this comment

corymhall Aug 3, 2022

Choose a reason for hiding this comment

rix0rrr Aug 8, 2022

Choose a reason for hiding this comment

stevehodgkiss Aug 22, 2022

Choose a reason for hiding this comment

corymhall Oct 10, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jul 28, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

plumdog commented Sep 8, 2022 • edited

plumdog commented Sep 9, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

corymhall commented Oct 10, 2022

corymhall left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 10, 2022

aws-cdk-automation commented Oct 10, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 10, 2022

plumdog commented Sep 8, 2022 •

edited