fix(ivs): Not a standard physical name pattern #24706
Conversation
github-actions
bot
added
the
valued-contributor
There was a problem hiding this comment.
@WinterYukky I am fine with the change around name => channelName since this is an alpha module, but the other change we can't accept.
| @@ -152,17 +153,19 @@ export class Channel extends ChannelBase { | |||
|
|
|||
| constructor(scope: Construct, id: string, props: ChannelProps = {}) { | |||
| super(scope, id, { | |||
| physicalName: props.name, | |||
| physicalName: props.channelName ?? Lazy.string({ | |||
There was a problem hiding this comment.
We should only generate a physical name for you if the resource requires that a name be set. In the case of CfnChannel the name is an optional property.
Use generated resource names, not physical names
This is instructing you to leave the name undefined so that CloudFormation will generate a physical name for you.
There was a problem hiding this comment.
Thank you very much for reviewing this PR @corymhall !!
Please tell me about the CDK physical name design. I understood physical name is generated by the CDK or CloudFormation. However, aws-ivs.Channel not generate a physical name both CDK and CloudFormation. So when we not provide physicalName then channel is created by CloudFormation with no name.
So, rather than having CloudFormation generate the physical names, you mean that we should leave the behavior of CloudFormation up to whether it generates the physical names or not when we do not provide them, right?
There was a problem hiding this comment.
ahhh sorry I didn't realize that CloudFormation will just leave the name blank. In that case I'm fine with this.
There was a problem hiding this comment.
@WinterYukky that is the case for playback key pair as well?
There was a problem hiding this comment.
@corymhall
Good that there is no discrepancy in the perception of the physical name design policy.
Yes, the same applies to playback key pairs.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
## Summary - change physical name of Props to `<cfnResource>Name` (e.g channelName) - generate physical name by CDK if not present ## Why need this change Because the alpha package `aws-ivs` does not follow standard CDK physical name conventions. CDK expects an auto-generated physical name to be used if the physical name is omitted. This will result in the "Use generated resource names, not physical names " of [Best practice](https://docs.aws.amazon.com/cdk/v2/guide/best-practices.html#best-practices-apps-names) can be complied with. However, the current `aws-ivs` package configuration does not follow that rule, so if omitted, the resource will be created without a physical name. Being created without a physical name is inconvenient for resource management, and many customers are forced to use physical names explicitly. Also, the standard CDK naming convention for the physical name property should be of the form `<cfnResource>Name` like bucketName, but the IVS package is just `name` and does not conform to the standard. ## Why changes now Resolving these issues would require breaking changes and should be resolved before migrating to the stable package. ## Related issues Closes none. BREAKING CHANGE: Renamed ChannelProps.name to ChannelProps.channelName * Renamed PlaybackKeyPairProps.name to PlaybackKeyPairProps.playbackKeyPairName * Channel now generates a physical name if one is not provided * PlaybackKeyPair now generates a physical name if one is not provided ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Summary
<cfnResource>Name(e.g channelName)Why need this change
Because the alpha package
aws-ivsdoes not follow standard CDK physical name conventions.CDK expects an auto-generated physical name to be used if the physical name is omitted. This will result in the "Use generated resource names, not physical names " of Best practice can be complied with. However, the current
aws-ivspackage configuration does not follow that rule, so if omitted, the resource will be created without a physical name. Being created without a physical name is inconvenient for resource management, and many customers are forced to use physical names explicitly.Also, the standard CDK naming convention for the physical name property should be of the form
<cfnResource>Namelike bucketName, but the IVS package is justnameand does not conform to the standard.Why changes now
Resolving these issues would require breaking changes and should be resolved before migrating to the stable package.
Related issues
Closes none.
BREAKING CHANGE: Renamed ChannelProps.name to ChannelProps.channelName
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license