aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

fix(core): make bundlingRequired minimatch matching the same as in aws-cdk CLI - take 2

Open stevehodgkiss opened this issue 3 years ago • 3 comments

Fixes https://github.com/aws/aws-cdk/issues/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:

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

stevehodgkiss avatar Jul 20 '22 10:07 stevehodgkiss

gitpod-io[bot] avatar Jul 20 '22 10:07 gitpod-io[bot]

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)

TheRealAmazonKendra avatar Jul 28 '22 23:07 TheRealAmazonKendra

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 https://github.com/aws/aws-cdk/issues/21925#issuecomment-1240011576

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 ?

plumdog avatar Sep 08 '22 11:09 plumdog

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.

plumdog avatar Sep 09 '22 15:09 plumdog

Added example-integ-test because I added an integ test for this separately via https://github.com/aws/aws-cdk/pull/21294

corymhall avatar Oct 10 '22 15:10 corymhall

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

mergify[bot] avatar Oct 10 '22 15:10 mergify[bot]

AWS CodeBuild CI Report

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

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

aws-cdk-automation avatar Oct 10 '22 16:10 aws-cdk-automation

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

mergify[bot] avatar Oct 10 '22 16:10 mergify[bot]