-
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 #2878
Conversation
…ral typing usages across the library).
…f the base class name.
e9babf4
to
773da1b
Compare
…)" This reverts commit f0d8127.
Oops, I over-eagerly merged this because I thought it was ready and was trying to get ahead of merge conflicts. Only later did I realize the PR was still under discussion, so I undid the merge. However, I can't seem to reopen the PR, so you're going to have to recreate it. Sorry! |
resourceName: this.physicalName.value, | ||
}, | ||
}); | ||
this.trailArn = resourceIdentifiers.arn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we enforce using this and not just trail.trailArn
? Seems voluntary... I am worried that most people will not know to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine; adding this later is a backwards-compatible change.
const myRepository = new Repository(stack, 'MyRepository', props).notify('myTrigger'); | ||
const myRepository = new Repository(stack, 'MyRepository', { | ||
repositoryName: PhysicalName.of('MyRepository'), | ||
}).notify('myTrigger'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify
? is this an API we like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not, but I don't feel like it's related to the physical name change.
Moved to #2894 after Rico rekt the PR 🙂 |
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:
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.