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.

Sign up for GitHub

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

Jump to bottom

fix(ivs): Not a standard physical name pattern #24706

Merged
merged 2 commits into from
Mar 22, 2023
Merged

fix(ivs): Not a standard physical name pattern #24706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e58586
fix(ivs): Not a standard physical name pattern
WinterYukky Mar 20, 2023
0f9bf75
Merge branch 'main' into fix/ivs-physical-name
mergify[bot] Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions packages/@aws-cdk/aws-ivs/lib/channel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as core from '@aws-cdk/core';
import { Lazy, Names } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnChannel } from './ivs.generated';
import { StreamKey } from './stream-key';
Expand Down Expand Up @@ -93,11 +94,11 @@ export interface ChannelProps {
readonly latencyMode?: LatencyMode;

/**
* Channel name.
* A name for the channel.
*
* @default - None
* @default Automatically generated name
*/
readonly name?: string;
readonly channelName?: string;

/**
* The channel type, which determines the allowable resolution and bitrate.
Expand Down Expand Up @@ -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

produce: () => Names.uniqueResourceName(this, { maxLength: 128, allowedSpecialCharacters: '-_' }),
}),
});

if (props.name && !core.Token.isUnresolved(props.name) && !/^[a-zA-Z0-9-_]*$/.test(props.name)) {
throw new Error(`name must contain only numbers, letters, hyphens and underscores, got: '${props.name}'`);
if (this.physicalName && !core.Token.isUnresolved(this.physicalName) && !/^[a-zA-Z0-9-_]*$/.test(this.physicalName)) {
throw new Error(`channelName must contain only numbers, letters, hyphens and underscores, got: '${this.physicalName}'`);
}

const resource = new CfnChannel(this, 'Resource', {
authorized: props.authorized,
latencyMode: props.latencyMode,
name: props.name,
name: this.physicalName,
type: props.type,
});

Expand Down
17 changes: 11 additions & 6 deletions packages/@aws-cdk/aws-ivs/lib/playback-key-pair.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as core from '@aws-cdk/core';
import { Lazy, Names } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnPlaybackKeyPair } from './ivs.generated';

Expand Down Expand Up @@ -35,9 +36,9 @@ export interface PlaybackKeyPairProps {
* An arbitrary string (a nickname) assigned to a playback key pair that helps the customer identify that resource.
* The value does not need to be unique.
*
* @default None
* @default Automatically generated name
*/
readonly name?: string;
readonly playbackKeyPairName?: string;
}

/**
Expand All @@ -54,15 +55,19 @@ export class PlaybackKeyPair extends PlaybackKeyPairBase {
public readonly playbackKeyPairFingerprint: string;

constructor(scope: Construct, id: string, props: PlaybackKeyPairProps) {
super(scope, id, {});
super(scope, id, {
physicalName: props.playbackKeyPairName ?? Lazy.string({
produce: () => Names.uniqueResourceName(this, { maxLength: 128, allowedSpecialCharacters: '-_' }),
}),
});

if (props.name && !core.Token.isUnresolved(props.name) && !/^[a-zA-Z0-9-_]*$/.test(props.name)) {
throw new Error(`name must contain only numbers, letters, hyphens and underscores, got: '${props.name}'`);
if (props.playbackKeyPairName && !core.Token.isUnresolved(props.playbackKeyPairName) && !/^[a-zA-Z0-9-_]*$/.test(props.playbackKeyPairName)) {
throw new Error(`playbackKeyPairName must contain only numbers, letters, hyphens and underscores, got: '${props.playbackKeyPairName}'`);
}

const resource = new CfnPlaybackKeyPair(this, 'Resource', {
publicKeyMaterial: props.publicKeyMaterial,
name: props.name,
name: props.playbackKeyPairName,
});

this.playbackKeyPairArn = resource.attrArn;
Expand Down
2 changes: 0 additions & 2 deletions packages/@aws-cdk/aws-ivs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@
},
"awslint": {
"exclude": [
"props-physical-name:@aws-cdk/aws-ivs.ChannelProps",
"props-physical-name:@aws-cdk/aws-ivs.StreamKeyProps",
"props-physical-name:@aws-cdk/aws-ivs.PlaybackKeyPairProps",
"from-method:@aws-cdk/aws-ivs.StreamKey",
"from-method:@aws-cdk/aws-ivs.PlaybackKeyPair"
]
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-ivs/test/integ.ivs.js.snapshot/aws-cdk-ivs.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "30.0.0",
"version": "31.0.0",
"files": {
"71d02f468f201d8c79d33a72cd8debac3b538f4e0efb492c6c06aca0055029f8": {
"fef396e89f5ce4eed5b6b04937e297b9abeab6a5db8cd808498a20aca8c61ef3": {
"source": {
"path": "aws-cdk-ivs.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "71d02f468f201d8c79d33a72cd8debac3b538f4e0efb492c6c06aca0055029f8.json",
"objectKey": "fef396e89f5ce4eed5b6b04937e297b9abeab6a5db8cd808498a20aca8c61ef3.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
32 changes: 22 additions & 10 deletions packages/@aws-cdk/aws-ivs/test/integ.ivs.js.snapshot/aws-cdk-ivs.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
{
"Resources": {
"PlaybackKeyPairBE17315B": {
"DefaultPropertiesPlaybackKeyPairaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaB5D4BE27": {
"Type": "AWS::IVS::PlaybackKeyPair",
"Properties": {
"PublicKeyMaterial": "-----BEGIN PUBLIC KEY-----\nMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAEHBm/D9UFf1z4czcAFuM7w+tstxxzoLVo\nfa1OT0gQjRYsy/YTcrKI5FS7ur3NZIcmiwqerr7dP0wSZjfEMNe82W1zWdkxHJ6Y\n73g9gZDxwGdjowZjEOIvAeH2Of6NeDOo\n-----END PUBLIC KEY-----"
}
},
"DefaultPropertiesChannelaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa201FBD46": {
"Type": "AWS::IVS::Channel",
"Properties": {
"Name": "aws-cdk-ivsDefaultPropertiesChannel-_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaEDEAEDA9"
}
},
"AllPropertiesPlaybackKeyPair96291E97": {
"Type": "AWS::IVS::PlaybackKeyPair",
"Properties": {
"Name": "IVSIntegrationTestPlaybackKeyPair",
"PublicKeyMaterial": "-----BEGIN PUBLIC KEY-----\nMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAEs6k8Xf6WyFq3yZXoup8G/gH6DntSATqD\nYfo83eX0GJCKxJ8fr09h9LP9HDGof8/bo66P+SGHeAARGF/O9WPAQVUgSlm/KMFX\nEPtPtOm1s0GR9k1ydU5hkI++f9CoZ5lM\n-----END PUBLIC KEY-----"
}
},
"Channel4048F119": {
"AllPropertiesChannel737C871D": {
"Type": "AWS::IVS::Channel",
"Properties": {
"Authorized": true,
Expand All @@ -16,39 +28,39 @@
"Type": "BASIC"
}
},
"StreamKey9F296F4F": {
"AllPropertiesStreamKey2A169FFE": {
"Type": "AWS::IVS::StreamKey",
"Properties": {
"ChannelArn": {
"Fn::GetAtt": [
"Channel4048F119",
"AllPropertiesChannel737C871D",
"Arn"
]
}
}
}
},
"Outputs": {
"PlaybackKeyPairArn": {
"AllPropertiesPlaybackKeyPairArn9C29D23B": {
"Value": {
"Fn::GetAtt": [
"PlaybackKeyPairBE17315B",
"AllPropertiesPlaybackKeyPair96291E97",
"Arn"
]
}
},
"ChannelArn": {
"AllPropertiesChannelArn97A102C5": {
"Value": {
"Fn::GetAtt": [
"Channel4048F119",
"AllPropertiesChannel737C871D",
"Arn"
]
}
},
"StreamKeyArn": {
"AllPropertiesStreamKeyArnB62C0761": {
"Value": {
"Fn::GetAtt": [
"StreamKey9F296F4F",
"AllPropertiesStreamKey2A169FFE",
"Arn"
]
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ivs/test/integ.ivs.js.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"30.0.0"}
{"version":"31.0.0"}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ivs/test/integ.ivs.js.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.0.0",
"version": "31.0.0",
"testCases": {
"ivs-test/DefaultTest": {
"stacks": [
Expand Down
2 changes: 1 addition & 1 deletion ...cdk/aws-ivs/test/integ.ivs.js.snapshot/ivstestDefaultTestDeployAssertDCE80D47.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.0.0",
"version": "31.0.0",
"files": {
"21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
"source": {
Expand Down
40 changes: 26 additions & 14 deletions packages/@aws-cdk/aws-ivs/test/integ.ivs.js.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "30.0.0",
"version": "31.0.0",
"artifacts": {
"aws-cdk-ivs.assets": {
"type": "cdk:asset-manifest",
Expand All @@ -17,7 +17,7 @@
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/71d02f468f201d8c79d33a72cd8debac3b538f4e0efb492c6c06aca0055029f8.json",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/fef396e89f5ce4eed5b6b04937e297b9abeab6a5db8cd808498a20aca8c61ef3.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
Expand All @@ -33,40 +33,52 @@
"aws-cdk-ivs.assets"
],
"metadata": {
"/aws-cdk-ivs/PlaybackKeyPair/Resource": [
"/aws-cdk-ivs/DefaultProperties/PlaybackKeyPair-_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "PlaybackKeyPairBE17315B"
"data": "DefaultPropertiesPlaybackKeyPairaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaB5D4BE27"
}
],
"/aws-cdk-ivs/Channel/Resource": [
"/aws-cdk-ivs/DefaultProperties/Channel-_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "Channel4048F119"
"data": "DefaultPropertiesChannelaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa201FBD46"
}
],
"/aws-cdk-ivs/StreamKey/Resource": [
"/aws-cdk-ivs/AllProperties/PlaybackKeyPair/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "StreamKey9F296F4F"
"data": "AllPropertiesPlaybackKeyPair96291E97"
}
],
"/aws-cdk-ivs/PlaybackKeyPairArn": [
"/aws-cdk-ivs/AllProperties/Channel/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "PlaybackKeyPairArn"
"data": "AllPropertiesChannel737C871D"
}
],
"/aws-cdk-ivs/ChannelArn": [
"/aws-cdk-ivs/AllProperties/StreamKey/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "ChannelArn"
"data": "AllPropertiesStreamKey2A169FFE"
}
],
"/aws-cdk-ivs/StreamKeyArn": [
"/aws-cdk-ivs/AllProperties/PlaybackKeyPairArn": [
{
"type": "aws:cdk:logicalId",
"data": "StreamKeyArn"
"data": "AllPropertiesPlaybackKeyPairArn9C29D23B"
}
],
"/aws-cdk-ivs/AllProperties/ChannelArn": [
{
"type": "aws:cdk:logicalId",
"data": "AllPropertiesChannelArn97A102C5"
}
],
"/aws-cdk-ivs/AllProperties/StreamKeyArn": [
{
"type": "aws:cdk:logicalId",
"data": "AllPropertiesStreamKeyArnB62C0761"
}
],
"/aws-cdk-ivs/BootstrapVersion": [
Expand Down
Loading