Skip to content
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

chore: Add note that assets are not account for in diff #1439

Merged
merged 1 commit into from
Dec 27, 2018

Conversation

RomainMuller
Copy link
Contributor

Fixes #1327
Related to #395


Pull Request Checklist

  • Testing
    • Unit test added
    • CLI change?: manually run integration tests and paste output as a PR comment
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@RomainMuller RomainMuller requested a review from a team as a code owner December 27, 2018 12:20
@@ -88,3 +93,7 @@ function buildLogicalToPathMap(template: cxapi.SynthesizedStack) {
}
return map;
}

function _hasAssets(stack: cxapi.SynthesizedStack) {
return Object.values(stack.metadata).find(entries => entries.find(entry => entry.type === cxapi.ASSET_METADATA) != null) != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious why you find it important to add != null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outer-most one is to force into a boolean context (for tsc to map it as a boolean), the middle one is out of habit.

@RomainMuller RomainMuller merged commit 36f69b6 into master Dec 27, 2018
@RomainMuller RomainMuller deleted the rmuller/diff-warning branch December 27, 2018 16:46
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
eladb pushed a commit that referenced this pull request Oct 3, 2019
The `NestedStack` construct is a special kind of `Stack`. Any resource defined within its scope will be included in a separate template from the parent stack. 

The template for the nested stack is synthesized into the cloud assembly but not treated as a deployable unit but rather as a file asset. This will cause the CLI to upload it to S3 and wire it's coordinates to the parent stack so we can reference its S3 URL.

To support references between the parent stack and the nested stack, we abstracted the concept of preparing cross references by inverting the control of `consumeReference` from the reference object itself to the `Stack` object. This allows us to override it at the `NestedStack` level (through `prepareCrossReference`) and mutate the token accordingly. 

When an outside resource is referenced within the nested stack, it is wired through a synthesized CloudFormation parameter. When a resource inside the nested stack is referenced from outside, it will be wired through a synthesized CloudFormation output. This works for arbitrarily deep nesting.

When an asset is referenced within a nested stack, it will be added to the top-level stack and wired through the asset parameter reference (like any other reference).

Fixes #239
Fixes #395
Related #3437 
Related #1439 
Related #3463
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add note that Assets aren't accounted for in cdk diff
3 participants