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

Better expose mutating properties of existing L1 resources #976

Closed
dstufft opened this issue Oct 19, 2018 · 1 comment
Closed

Better expose mutating properties of existing L1 resources #976

dstufft opened this issue Oct 19, 2018 · 1 comment
Assignees
Labels
feature-request A feature should be added or improved. package/cfn Related to the CFN layer (L1)

Comments

@dstufft
Copy link
Contributor

dstufft commented Oct 19, 2018

Currently if you're using the L1 resources, and you want to mutate a property, you need to call addPropertyOverride with a property name and the new value, so something like (in Python, ignore the naming):

import aws_cdk.aws_s3.cloudformation as s3_cloudformation
import aws_cdk.cdk as cdk

app = cdk.App()
stack = cdk.Stack(app, "Foo")

bucket = s3_cloudformation.BucketResource(
    stack, "S3Bucket", {"accessControl": "PublicRead"}
)
bucket.addPropertyOverride("accessControl", "BucketOwnerRead")

app.run()

Except that doesn't actually work, because I (mistakenly) assumed that there would be consistency between addPropertyOverride and the props being passed into the BucketResource. Worse than not actually working, it silently does the wrong thing, and produces output like this:

Resources:
    S3Bucket:
        Type: 'AWS::S3::Bucket'
        Properties:
            AccessControl: PublicRead
            accessControl: BucketOwnerRead

Thankfully, CloudFormation itself saves us here and it fails with an error saying that it has an unsupported property accessControl.

However, that's still a pretty poor behavior since a person who has made this mistake won't find out until deploy time that they've done something wrong. If someone is trying to do the right thing and have code review and/or tests prior to merging an infrastructure change into their code base, they're likely to miss this and break their mainline branch from being able to be deployed until they use the correct spelling.

This has a related problem, where the actual names that someone should be passing into addPropertyOverride are not very easily discoverable. You have to just sort of know that they're the same ones in the ResourceProps (and know what munging needs to be done) or you have to look up all of the attributes on the cloudformation documentation. It would be really great to increase the ergonomics of mutating (or even just accessing) these properties.

One possible suggestion is to leave addPropertyOverride alone, and leave it there solely to handle the case of a new property having been added since the L1 construct was last generated (though we'd likely want to rename it to something like addRawPropertyOverride or addUnknownPropertyOverride) and then add explicit getter/setter methods to the generated L1 constructs. That way the above could have been written as:

bucket = s3_cloudformation.BucketResource(
    stack, "S3Bucket", {"accessControl": "PublicRead"}
)
bucket.accessControl = "BucketOwnerRead"

In this case, we can both ensure that our naming is consistent AND that if someone typo'd the name or used the wrong name, that they would get an error immediately. That would be a pretty solid win in the ergonomics department in my opinion.

Of course we could also just eliminate addPropertyOverride completely after we do the above. It depends on how much we care about supporting properties for L1 resources that were unknown at generation time. Personally I feel like with release automation it should be possible to keep the L1 constructs up to date (similarly to how the SDKs are done) so that the latest release of the CDK always has all of the relevant properties generated. In that case, it would make sense not to add the addPropertyOverride method at all.

@eladb
Copy link
Contributor

eladb commented Oct 20, 2018

addOverride and addPropertyOverride are the "last resort" and allow the user to basically "patch" the resulting CloudFormation template in case L1 doesn't suite their needs. That's why the names of properties at that layer are not generated to be idiomatic for each language but rather must adhere to the CloudFormation raw names.

In most cases, users should be able to either just use L2 or, if they need to "escape" from L2, they should be able to use L1's propertyOverrides, which are strongly typed and use the idiomatic naming.

bucket.propertyOverrides.accessControl = 'BucketOwnerRead';

@eladb eladb added enhancement package/cfn Related to the CFN layer (L1) labels Dec 17, 2018
@srchase srchase added feature-request A feature should be added or improved. and removed enhancement labels Jan 3, 2019
@eladb eladb self-assigned this Aug 12, 2019
@eladb eladb closed this as completed Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. package/cfn Related to the CFN layer (L1)
Projects
None yet
Development

No branches or pull requests

3 participants