-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat: physical names in the entire Construct Library #2894
Conversation
Continued from #2878 |
I think you're correct on the resources you skipped, except I'm also unsure about a copule. I think these are just "usual" physical names, still. Am I wrong?
|
Pre-emptive approval but please check those resources. |
The problem with the @aws-config Rules is that the linter rule checks for the name of the property. If the class is called @rix0rrr opinion on this? (and anyone else, of course) |
e42b26a
to
f9f19ad
Compare
That shouldn't be a reason to not make them PhysicalNames though. It might mean we have to suppress the linter rules for those classes, OR we fix the linter to not look at the class name as a prefix, but the resource name as a prefix. |
f9f19ad
to
025d8f3
Compare
Changed the linter to take into account the resource name instead of the class name in the newest version. |
This PR changes the xyzName attribute of all Resources in the Construct Library to be of type PhysicalName, and adds an AWS linter rule that checks ensures conformance. The name of the property can be any ending substring of the base name of the class with the Name suffix - for example, if my resource is AwesomeFoobar, the name can be foobarName or awesomeFoobarName (that's because we often have Specialized1AwesomeFoobar and Specialized2AwesomeFoobar in our library, that share a base prop interface). BREAKING CHANGE: all <resource>Name attributes have their type changed from string to cdk.PhysicalName.
025d8f3
to
2f7a762
Compare
This PR changes the
xyzName
attribute of all Resources in the Construct Library to be of type PhysicalName,and adds an AWS linter rule that checks ensures conformance.
The name of the property can be any ending substring of the base name of the class with the
Name
suffix - for example, if my resource is
AwesomeFoobar
, the name can befoobarName
orawesomeFoobarName
(that's because we often haveSpecialized1AwesomeFoobar
andSpecialized2AwesomeFoobar
in our library, that share a base prop interface).There were a few interesting cases in the library which I didn't change, as I wasn't sure of the semantics of the resources, or they would require class name changes. Would appreciate some guidance there. These were:
@aws-ssm: ParameterOptions.nameEDIT: changed toPhysicalName
according to @rix0rrr 's suggestion below@aws-config: CustomRuleProps.ruleName@aws-config: ManagedRuleProps.ruleName@aws-config: AccessKeysRotatedProps.ruleName@aws-config: CloudFormationStackNotificationCheckProps.ruleName@aws-config: CloudFormationStackDriftDetectionCheckProps.ruleNameEDIT: changed the linter to take the resource name instead of the class name, and now @aws-config has been changed as well
BREAKING CHANGE: all resourceName attributes have their type changed from string to cdk.PhysicalName.
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.