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

Implement constructs extensions #200

Merged
merged 5 commits into from
May 30, 2022
Merged

Conversation

fredericbarthelet
Copy link
Collaborator

Fixes #191

@t-richard
Copy link
Contributor

I love the idea!

I don't have a clear idea of what the ideal solution would be but here are some thoughts to get the discussion started around this.

Do we want to expose Cloudformation properties? Or would it be possible to expose underlying CDK properties? I guess the latter is harder because not everything is a property, some things need to be created as separate objects or as method calls.

From a Lift developer perspective, wouldn't it be more friendly to have a generic way to expose and merge config built in the provider instead of each individual construct?

Would this be as resources.extensions in Serverless framework? I now that in sls, the extensions are only replacing keys at the first level. This would be problematic for Lift as some Cloudformation resources are poorly designed on AWS side like Cloudfront distributions that have a single DistributionConfig key in properties.
Overriding the static website or server-side website would get way harder as you would need to redifine every property to make it work with undermines the benefits of using Lift.

@fredericbarthelet fredericbarthelet marked this pull request as ready for review May 24, 2022 17:29
@fredericbarthelet
Copy link
Collaborator Author

fredericbarthelet commented May 24, 2022

Would this be as resources.extensions in Serverless framework? I now that in sls, the extensions are only replacing keys at the first level. This would be problematic for Lift as some Cloudformation resources are poorly designed on AWS side like Cloudfront distributions that have a single DistributionConfig key in properties. Overriding the static website or server-side website would get way harder as you would need to redifine every property to make it work with undermines the benefits of using Lift.

Thanks for your feedback @t-richard :) I made Lift's API to provide extensions entry point as simple to enrich as possible. I now completed first version of this PR for you and @mnapoli to have a deeper look on the internals. Additional entry point can easily be added for the extensions as use cases pop up.

For exemple, if you want to provide for Lift users at later stage an option to extend the OriginRequestPolicy of the underlying CloudFormation resource within server-side-website construct, you'll "just" have to add the following line in the extend method of the construct :

    extend(): Record<string, CfnResource> {
        return {
            distribution: this.distribution.node.defaultChild as CfnDistribution,
            originRequestPolicy: this.originRequestPolicy.node.defaultChild a CfnCachePolicy
        };
    }

More complex selector that the defaultChild of each CDK node can be used to reference the underlying in depth resources of the CloudFormation template. Would welcome any PR in a second stage implementing those to ensure no extensions requires a whole bunch of Lift best practice to be reimplemented manually ;)

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

That looks awesome, and the docs too!

Thanks 🎉 @fredericbarthelet


| Extension key | CloudFormation resource | CloudFormation documentation |
|--------------- |------------------------------- |------------------------------------------------------------------------------------------------------------------------ |
| distribution | AWS::CloudFront::Distribution | [Link](https://docs.aws.amazon.com/fr_fr/AWSCloudFormation/latest/UserGuide/aws-resource-cloudfront-distribution.html) |
Copy link

Choose a reason for hiding this comment

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

hello,

may I ask a question, more than a comment?
For this type of builder, we will also create an S3 bucket, will it be possible to adjust it, like the "storage" builder, or will we only be able to adjust the distribution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely possible :) I'll merge this one, if you want to provide a PR on the basis of this new code base to implement the storage extension on this construct, you're welcome to :)

Copy link

Choose a reason for hiding this comment

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

I would love to, but I can't compile the project properly, nor can I run the unit tests without errors on my side (on the branch master). So it's hard to try implementations :/

@fredericbarthelet fredericbarthelet merged commit 0ab2033 into master May 30, 2022
@fredericbarthelet fredericbarthelet deleted the construct-extensions branch May 30, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement resources extensions to allow further customization of Lift Constructs underlying CFN resources
4 participants