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

fix(cx-api): cannot detect CloudAssembly across different libraries #32998

Merged
merged 6 commits into from
Jan 21, 2025

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Jan 17, 2025

Reason for this change

We are publishing the cx-api package twice: Once as a standalone package @aws-cdk/cx-api and once as part of the construct library under aws-cdk-lib/cx-api. The code is copied during the release and the same versions of the packages will have the same code.

However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from cx-api is CloudAssembly - the result of app.synth(). Previously a package had to take a dependency on the very large aws-cdk-lib just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused @aws-cdk/cx-api package.

Description of changes

This adds the same mechanism to CloudAssembly to detect cross-library compatibility, that we already use for constructs like Stack or App. In TypeScript, it's now possible for a consuming package to receive an object from either package and check at runtime if it satisfies the requirements.

We cannot get around type checking with this. Instead we introduce a new type ICloudAssembly into the Cloud Assembly Schema package (cdklabs/cloud-assembly-schema#133). This interface only declares a single property: directory. Consumers can use this type to indicate where they would like to receive a CloudAssembly. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new CloudAssembly from the directory.

Because the CloudAssembly in cxapi implements the new interface, this approach will work in all jsii languages. In TypeScript it's even compatible with older version of aws-cdk-lib. Jsii language will only support this going forward.

Allowed breaking changes

weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry
weakened:aws-cdk-lib.cx_api.MetadataEntryResult

This PR updates the version of @aws-cdk/cloud-assembly-schema to make new of the new interface.
However the update also includes a change to MetadataEntry which was introduced in cdklabs/cloud-assembly-schema#121. That change is weakening a type, because in #31041, the CDK started emitting booleans and numbers as metadata values.

But since these types weren't officially declared in the schema, jsii runtime type checking failed to load them.
The fix was to officially extend the type union to include boolean and number primitive values.
This is considered breaking, because when used as an output any consuming code will now need to account for the possibility of the value being a boolean or number. In static languages, the type would already have been treated as a generic Object with required runtime checks.

Describe any new or updated permissions being added

n/a

Description of how you validated changes

Unit tests.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team January 17, 2025 21:40
@github-actions github-actions bot added the p2 label Jan 17, 2025
@mrgrain mrgrain changed the title fix(cx-api): cannot detect CloudAssembly across different library ver… fix(cx-api): cannot detect CloudAssembly across different libraries Jan 17, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 17, 2025
aws-cdk-automation

This comment was marked as outdated.

@mrgrain mrgrain force-pushed the mrgrain/feat/toolkit/assembly-return branch from 2680942 to ed53139 Compare January 17, 2025 21:54
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.46%. Comparing base (6d834c0) to head (21438b5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #32998   +/-   ##
=======================================
  Coverage   81.46%   81.46%           
=======================================
  Files         224      224           
  Lines       13748    13748           
  Branches     2412     2412           
=======================================
  Hits        11200    11200           
  Misses       2273     2273           
  Partials      275      275           
Flag Coverage Δ
suite.unit 81.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 80.85% <ø> (ø)
packages/aws-cdk-lib/core 82.10% <ø> (ø)

mergify bot pushed a commit to cdklabs/cloud-assembly-schema that referenced this pull request Jan 20, 2025
### Issue

Relates to aws/aws-cdk#32998

### Reason for this change

We are publishing the `cx-api` package twice: Once as a standalone package `@aws-cdk/cx-api` and once as part of the construct library under `aws-cdk-lib/cx-api`. The code is copied during the release and the same versions of the packages will have the same code. 

However this makes it difficult for other packages to take a type dependency on types from this package. The most common class that's used from `cx-api` is `CloudAssembly` - the result of `app.synth()`. Previously a package had to take a dependency on the very large `aws-cdk-lib` just to use a single type. It would be better if other packages could instead depend on the smaller, much more focused `@aws-cdk/cx-api` package.


### Description of changes

In aws/aws-cdk#32998 we are adding a mechanism to detect cross-library compatibility of the `CloudAssembly` class. **However, that alone doesn't help us with type checking.** Instead we introduce a new type `ICloudAssembly` into the Cloud Assembly Schema. This interface only declares a single property: `directory`. Consumers can use this type to indicate where they would like to receive a `CloudAssembly`. They can then use runtime code to either confirm a provided object already satisfies the requirements or fallback to creating a new `CloudAssembly` from the directory.
@mrgrain mrgrain force-pushed the mrgrain/feat/toolkit/assembly-return branch 2 times, most recently from 8ca8b49 to ededa21 Compare January 20, 2025 19:13
@mrgrain mrgrain marked this pull request as ready for review January 20, 2025 19:14
@mrgrain mrgrain requested a review from a team as a code owner January 20, 2025 19:14
@mrgrain mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 20, 2025
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 20, 2025

Applied pr-linter/exempt-integ-test because change in cx-api was not to a construct.

@mrgrain mrgrain force-pushed the mrgrain/feat/toolkit/assembly-return branch from ededa21 to 847ef78 Compare January 20, 2025 19:24
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 20, 2025 19:24

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

@mrgrain mrgrain force-pushed the mrgrain/feat/toolkit/assembly-return branch from 847ef78 to d6d3085 Compare January 20, 2025 20:58
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Jan 21, 2025
@mrgrain mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Jan 21, 2025
Copy link
Contributor

mergify bot commented Jan 21, 2025

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

mergify bot commented Jan 21, 2025

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 mergify bot merged commit 94ba772 into main Jan 21, 2025
19 checks passed
@mergify mergify bot deleted the mrgrain/feat/toolkit/assembly-return branch January 21, 2025 10:53
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants