search

pulumi / pulumi-cdk

Pulumi/CDK Interop Library
Apache License 2.0
61 stars 5 forks source link

Escape double-colons (::) #85

Open karakter98 opened 1 year ago

karakter98 commented 1 year ago

Escape double-colons (::) from AWS custom resource naming convention, so the resource URNs are compatible with Pulumi's format.

Allows users to define their own dynamic providers for custom resources, which partially solves #60.

With these minimal changes, I was able to implement a generic dynamic provider for custom resources, which seems to work fine for S3BucketDeployment and S3AutoDeleteObjects resources. I'll brush it up a bit and will open another PR.

github-actions[bot] commented 1 year ago

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

iwahbe commented 1 year ago

Hi @karakter98. Thanks for the PR! It would be really helpful if you could post an example of what works with this change, but didn't work without it. A quick test would be even better.

iwahbe commented 1 year ago

/run-acceptance-tests

github-actions[bot] commented 1 year ago

Please view the PR build - https://github.com/pulumi/pulumi-cdk/actions/runs/6005610699

karakter98 commented 1 year ago

@iwahbe I tried to write a test, but it seems the pulumi runtime isn't actually called to register the resource, so the URN error never shows up in the test file. I believe it's because of the mocks, but I'm not familiar enough with the codebase to figure it out further.

What we can do now and couldn't do before: register custom resources from CDK that contain :: in their names, like Custom::S3AutoDeleteObjects from the cdk Bucket class.

karakter98 commented 1 year ago

A quick example:

const bucket = new Bucket(this, "Bucket", {
    enforceSSL: true,
    removalPolicy: RemovalPolicy.DESTROY,
    autoDeleteObjects: true,
});

new BucketDeployment(this, "Deployment", {
    sources: [Source.asset(path.join("..", "build", "web"))],
    destinationBucket: bucket,
    retainOnDelete: false,
});

Both the S3AutoDeleteObjects and S3BucketDeployment custom resources created by the above code can't be registered by Pulumi, because of double-colons in the resource names (of the form Custom::S3AutoDeleteObjects or similar). This clashes with Pulumi's URN format when it tries to register the resources.

iwahbe commented 1 year ago

Thanks for the example @karakter98. I have asked someone familiar with pulumi-ckd to take a look at your PR.

karakter98 commented 1 year ago

@lukehoban Yes, I'm currently using this to deploy the frontend for a new project, so I can confirm it allows the use of custom resources.

The actual complete example is a bit complicated, because it uses:

I think it makes sense to split this into multiple PRs, because so much setup is needed on the user side that the test code would be pretty huge.

I'll test out your suggestion with %3A encoding and see if Pulumi accepts that format, thanks for the idea!

lukehoban commented 1 year ago

I think it makes sense to split this into multiple PRs, because so much setup is needed on the user side that the test code would be pretty huge.

Got it. Yeah - I tried out the original example and ran into all those same problems as well. I’ll open some issues to track the remaining things we’ll need to address here. If you already have PRs in progress, great!

github-actions[bot] commented 1 year ago

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

karakter98 commented 1 year ago

@lukehoban I changed the encoding according to your feedback, can confirm Pulumi registers the names correctly with the %3A (ran pulumi preview on the project I'm working on without errors)

iwahbe commented 1 year ago

/run-acceptance-tests

github-actions[bot] commented 1 year ago

Please view the PR build - https://github.com/pulumi/pulumi-cdk/actions/runs/6051638879

karakter98 commented 1 year ago

Is this fine to be merged? I already opened other PRs and am ready to start working on the dynamic provider PR for custom resources, but this needs to be merged first

karakter98 commented 1 year ago

@lukehoban could you take a look when you get some time?

github-actions[bot] commented 8 months ago

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR