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

aws-cdk: cx-api installed twice by NPM #17974

Closed
mnapoli opened this issue Dec 12, 2021 · 4 comments · Fixed by #19511
Closed

aws-cdk: cx-api installed twice by NPM #17974

mnapoli opened this issue Dec 12, 2021 · 4 comments · Fixed by #19511
Labels
@aws-cdk/cx-api Related to the Cloud Executable API bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@mnapoli
Copy link
Contributor

mnapoli commented Dec 12, 2021

What is the problem?

When requiring aws-cdk and aws-cdk-lib in a project, then the cx-api package gets installed twice:

  • once as a dependency of aws-cdk
  • once inside aws-cdk-lib

That causes a problem similar to #14468 because of this bit:

function isAssetManifestArtifact(art: cxapi.CloudArtifact): art is cxapi.AssetManifestArtifact {
return art instanceof cxapi.AssetManifestArtifact;
}

The instanceof fails because of the class duplication (again, similar thing happened in #14468, see this comment: #14468 (comment)).

I signaled this with CDK v1 here: #14468 (comment) but the same problem happens with CDK v2.

Note: I am requiring both of those package to deploy with the CDK programmatically, not deploying via the CLI. BTW let me know if that is completely not meant to be done that way.

Reproduction Steps

Package.json:

{
  "dependencies": {
    "aws-cdk": "^2.1.0",
    "aws-cdk-lib": "^2.1.0",
    "aws-sdk": "^2.1046.0"
  }
}

JS file:

        const app = new App();
        const stack = new Stack(app, 'test-stack', {
            env: {
                region: 'us-east-1',
                name: "dev",
            }
        });
        new Queue(stack, 'Queue');

        const stackArtifact = app.synth().getStackByName('test-stack');

        const sdkProvider = new SdkProvider(new CredentialProviderChain(), 'us-east-1');
        const cloudFormation = new CloudFormationDeployments({ sdkProvider });
        await cloudFormation.deployStack({
            stack: stackArtifact,
        });

What did you expect to happen?

isAssetManifestArtifact() should return true. And the deployment should succeed.

What actually happened?

isAssetManifestArtifact() returns false, caused some assets not to be uploaded to S3 (namely the CloudFormation template).

That causes the deployment to fail because the CloudFormation deployment is looking for a template on S3 that doesn't exist (it was never uploaded there).

CDK CLI Version

Not using the CDK CLI

Framework Version

No response

Node.js Version

17.0.1

OS

macOS

Language

Typescript

Language Version

4.1.2

Other information

I have no idea how we could make that instanceof more robust.

BUT even if we did, what if there are other instanceof tests in the codebase? (#14468 tells us that this isn't an isolated problem)

Maybe we should fix the root problem: how can we make sure cx-api (or any other dependency) is installed once in projects that require aws-cdk and aws-cdk-lib?

@mnapoli mnapoli added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 12, 2021
@github-actions github-actions bot added the @aws-cdk/cx-api Related to the Cloud Executable API label Dec 12, 2021
@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Dec 13, 2021
@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 16, 2021

Here is the patch I did:

function isAssetManifestArtifact(art) {
+    return art.constructor.name === 'AssetManifestArtifact';
-    return art instanceof cxapi.AssetManifestArtifact;
}

I could send a pull request, but I'm not sure:

  • if it's robust enough
  • if it's wise enough to fix locally, where it might need to be fixed globally -> is it a scenario where "peerDependencies" might be useful?

@rix0rrr rix0rrr removed their assignment Feb 9, 2022
mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 22, 2022
@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 22, 2022

I have opened #19511 to fix this because this is a huge blocker.

Again, this happens when package.json requires:

{
  "dependencies": {
    "aws-cdk": "^2.1.0",
    "aws-cdk-lib": "^2.1.0"
  }
}

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 22, 2022

deploy with the CDK programmatically

There's the rub. We don't support doing this. If you use the CLI as intended, this problem will not occur.

mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 22, 2022
mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 23, 2022
mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 23, 2022
mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 23, 2022
mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 23, 2022
mnapoli added a commit to mnapoli/aws-cdk that referenced this issue Mar 23, 2022
@mergify mergify bot closed this as completed in #19511 Mar 29, 2022
mergify bot pushed a commit that referenced this issue Mar 29, 2022
Fixes #17974

Follow-up of #14468 

This follows the implementation of aws/constructs#955 to be more robust.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this issue Apr 27, 2022
Fixes aws#17974

Follow-up of aws#14468 

This follows the implementation of aws/constructs#955 to be more robust.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/cx-api Related to the Cloud Executable API bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants