Skip to content

Conversation

@mpuhacz
Copy link

@mpuhacz mpuhacz commented Sep 29, 2021

Fixes: #16713


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 29, 2021

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 0934656
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@mpuhacz
Copy link
Author

mpuhacz commented Sep 29, 2021

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 0934656
  • Result: FAILED
  • Build Logs (available for 30 days)

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

A not related test is failing. Can we restart the test job?

@aws-cdk/custom-resources: FAIL test/aws-custom-resource/aws-custom-resource-provider.test.js (196.634 s)
@aws-cdk/custom-resources:   â—� installs the latest SDK
@aws-cdk/custom-resources:     : Timeout - Async callback was not invoked within the 60000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 60000 ms timeout specified by jest.setTimeout.Error:
@aws-cdk/custom-resources:       420 | });
@aws-cdk/custom-resources:       421 |
@aws-cdk/custom-resources:     > 422 | test('installs the latest SDK', async () => {
@aws-cdk/custom-resources:           | ^
@aws-cdk/custom-resources:       423 |   const tmpPath = '/tmp/node_modules/aws-sdk';
@aws-cdk/custom-resources:       424 |
@aws-cdk/custom-resources:       425 |   // Symlink to normal SDK to be able to call AWS.setSDK()
@aws-cdk/custom-resources:       at new Spec (../../../node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
@aws-cdk/custom-resources:       at Object.<anonymous> (test/aws-custom-resource/aws-custom-resource-provider.test.ts:422:1)
@aws-cdk/custom-resources: =============================== Coverage summary ===============================
@aws-cdk/custom-resources: Statements   : 93.55% ( 334/357 )
@aws-cdk/custom-resources: Branches     : 91.98% ( 218/237 )
@aws-cdk/custom-resources: Functions    : 94.91% ( 56/59 )
@aws-cdk/custom-resources: Lines        : 93.44% ( 328/351 )
@aws-cdk/custom-resources: ================================================================================
@aws-cdk/custom-resources: Test Suites: 1 failed, 4 passed, 5 total

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 29, 2021

Thanks, good catch.

I'm curious how you validated this change, and I'd also like to see a test added that shows the difference.

@mpuhacz
Copy link
Author

mpuhacz commented Sep 29, 2021

@rix0rrr
I was testing the change manually using the code from issue #16713 (see, Reproduction Steps).
Happy to add tests. I didn't find one for asset-staging.ts, would creating a new one in aws-cdk/core/test/asset-staging.test.ts be alright?

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 4, 2021

would creating a new one in aws-cdk/core/test/asset-staging.test.ts be alright?

That seems excellent, thanks

@peterwoodworth peterwoodworth changed the title fix(core): AssetStaging - make skip flag use stack id instead of stack name fix(core): AssetStaging - make skip flag use stack id instead of stack name Oct 21, 2021
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Oct 21, 2021
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Changes requested according to discussion

// Check if we actually have to bundle for this stack
const bundlingStacks: string[] = this.node.tryGetContext(cxapi.BUNDLING_STACKS) ?? ['*'];
skip = !bundlingStacks.find(pattern => minimatch(Stack.of(this).stackName, pattern));
skip = !bundlingStacks.find(pattern => minimatch(Stack.of(this).node.id, pattern));
Copy link
Contributor

Choose a reason for hiding this comment

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

Using node.id drops the stage name

+ another issue is that bundlingStacks is of the form Stage/Stack while stackName is of the form Stage-Stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rix0rrr rix0rrr added bug This issue is a bug. p1 and removed @aws-cdk/core Related to core CDK functionality labels Mar 4, 2022
@aws-cdk-automation
Copy link
Collaborator

This PR has been in CHANGES REQUESTED for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it.

@github-actions
Copy link
Contributor

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@github-actions github-actions bot added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Apr 20, 2022
@github-actions github-actions bot closed this Apr 20, 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. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(core): AssetStaging skips stacks with custom stackName specified

4 participants