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(ivs): Not a standard physical name pattern #24706

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

WinterYukky
Copy link
Contributor

@WinterYukky WinterYukky commented Mar 20, 2023

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 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

@github-actions github-actions bot added the p2 label Mar 20, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team March 20, 2023 17:42
@github-actions github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Mar 20, 2023
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@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({
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh sorry I didn't realize that CloudFormation will just leave the name blank. In that case I'm fine with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WinterYukky that is the case for playback key pair as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@corymhall
Good that there is no discrepancy in the perception of the physical name design policy.
Yes, the same applies to playback key pairs.

image

image

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 0f9bf75
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 7d17fe3 into aws:main Mar 22, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

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).

@WinterYukky WinterYukky deleted the fix/ivs-physical-name branch March 23, 2023 00:02
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
## 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants