-
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(assets): add missing SAM asset metadata information #17591
Conversation
…data to indicate the asset metadata original path before staging
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
Pull request has been modified.
Pull request has been modified.
Thank you for contributing! Your pull request will be updated from master 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 |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
@@ -191,6 +207,8 @@ export class Asset extends CoreConstruct implements cdk.IAsset { | |||
// points to a local path in order to enable local invocation of this function. | |||
resource.cfnOptions.metadata = resource.cfnOptions.metadata || { }; | |||
resource.cfnOptions.metadata[cxapi.ASSET_RESOURCE_METADATA_PATH_KEY] = this.assetPath; | |||
resource.cfnOptions.metadata[cxapi.ASSET_RESOURCE_METADATA_ORIGINAL_PATH_KEY] = this.originalAssetPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we injecting absolute paths into templates? Doesn't that break build determinism across machines?
See https://github.com/cdklabs/construct-hub/pull/648/files#r764446322 (copy @Chriscbr).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't afford for this to be an absolute path because it means that CDK output will differ between systems (e.g. where the project directory is, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking the fix here: #17706
Following up on issue aws#14593 The integration with SAM tool requires to have some more info about the Assets. SAM needs to know if the Asset was already bundled or not, and what is the original asset path before staging. This change is to add the following assets metadata: - aws:asset:is-bundled - aws:asset:original-path ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Following up on issue #14593
The integration with SAM tool requires to have some more info about the Assets. SAM needs to know if the Asset was already bundled or not, and what is the original asset path before staging.
This change is to add the following assets metadata:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license