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(aws-ssm): correctly emit Association.Name #854

Closed
wants to merge 2 commits into from

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 5, 2018

The rename used to happen in the cfnspec, which helped the model
generate correct names, but the code generator would lose the ability to
access the original name, and so it could not be emitted correctly.

Add a property renaming feature to the 'cfn2ts' code generator
(configured via a special entry in package.json).

Fixes #852.

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

The rename used to happen in the cfnspec, which helped the model
generate correct names, but the code generator would lose the ability to
access the original name, and so it could not be emitted correctly.

Add a property renaming feature to the 'cfn2ts' code generator
(configured via a special entry in package.json).

Fixes #852.
@rix0rrr rix0rrr requested review from RomainMuller and eladb October 5, 2018 10:58
@eladb
Copy link
Contributor

eladb commented Oct 5, 2018

Not sure I like this and it makes me wonder: maybe we should just get rid of the "name" => "xxxName" renames altogether in L1. The reason we are actually doing these renames was that the construct ID used to be a "name" prop, so if an L1 had a Name property, they would conflict.

However, now construct IDs are not a prop, and we technically don't need these renames at all.

}
if (finalNames.has(propName)) {
throw new Error(`Oh gosh, there was already a property named ${propName}!`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for preserving the spirit

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 8, 2018

The reason we are actually doing these renames was that the construct ID used to be a "name" prop, so if an L1 had a Name property, they would conflict.

True, that was the original reason. At the same time, I do like the transformation we're doing right now, if only to make the object model more consistent. The CloudFormation object model isn't always consistent in the names it uses, but I'd rather have our model be consistent.

For example (in no particular order, just random sampling from the CloudFormation model):

  • AWS::Kinesis::Stream, AWS::CodeCommit::Repository: property Name.
  • AWS::CodeDeploy::Application, property ApplicationName, AWS::DynamoDB::Table, property TableName.
  • Not to mention AWS::SSM::Association which has both Name and AssociationName.

And at the same time, when we generate {GetAtt} and {Ref} getters, we do always add in the resource type into the name (for example { Fn::GetAtt: [Bucket, Arn] } -> bucket.bucketArn (as opposed to bucket.arn)). For consistency's sake we do use the resource type in there a lot, and I don't mind the explicit verbosity here.

I'm aware we could be providing this uniformity at the L2 level as well... but this is just such a convenient place to add it. I'm torn.

@eladb
Copy link
Contributor

eladb commented Oct 8, 2018

I think there is value in staying 1:1 with the CFN resource spec at L1. It makes it easier for people to navigate this layer, find help and in general have less magic. I vote for removing this sugar...

@RomainMuller
Copy link
Contributor

Regarding the name prop... There are other conflict cases that we very much ignore at the moment, though... What will happen when some CFN resource gets a Path attribute is going to be not pretty.

@eladb
Copy link
Contributor

eladb commented Oct 8, 2018

@RomainMuller This is about the resource properties (props) not attributes, which are implemented as the properties of the construct (and we prefix the resource name to them all).

@RomainMuller
Copy link
Contributor

Oh, duh. 🤦🏻‍♂️

@rix0rrr rix0rrr closed this Oct 8, 2018
@eladb
Copy link
Contributor

eladb commented Oct 10, 2018

@rix0rrr do you want me to pick this up?

@eladb eladb deleted the huijbers/ssm-document-name branch November 19, 2018 12:31
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Framework: remove property renames for CloudFormation constructs
4 participants