feat(ivs): support recording configuration for channel #31899

mazyu36 · 2024-10-25T08:45:16Z

Issue # (if applicable)

Closes #31780.

Reason for this change

To use recording configuration for IVS channel.

Description of changes

Add RecordingConfiguration Construct.
Add recordingConfiguration property to the Channel.

Description of how you validated changes

Add unit tests and integ test.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

paulhcsun

Thanks for the contribution @mazyu36!! I've just left a few comments on changing a couple property types to be enum-like classes instead of enums with runtime error handling. Let me know what you think!

packages/@aws-cdk/aws-ivs-alpha/README.md

paulhcsun · 2024-11-05T00:36:03Z

packages/@aws-cdk/aws-ivs-alpha/lib/recording-configuration.ts

+  /**
+   * A list of which renditions are recorded for a stream.
+   * This property must be set when `renditionSelection` is `RenditionSelection.CUSTOM`.
+
+   * @default - no resolution selected
+   */
+  readonly renditions?: Resolution[];


I would prefer to avoid having this as a separate property that is dependent on the value of renditionSelection and instead prefer to use an enum-like class for the type of renditionSelection where if we set it to RenditionSelection.Custom() then it would take in a Resolution[] as a required parameterand if we setRenditionSelection.All()then it would not include a parameter forrenditions` making it impossible to set the invalid pairing of properties.

I've created RenditionConfiguration class.

paulhcsun · 2024-11-05T00:37:53Z

packages/@aws-cdk/aws-ivs-alpha/lib/recording-configuration.ts

+  private validateRenditionSettings(): undefined {
+    if (this.props.renditionSelection === RenditionSelection.CUSTOM && !this.props.renditions) {
+      throw new Error('`renditions` must be provided when \`renditionSelection\` is `RenditionSelection.CUSTOM`.');
+    }
+
+    if (this.props.renditionSelection !== RenditionSelection.CUSTOM && this.props.renditions) {
+      throw new Error(`\`renditions\` can only be set when \`renditionSelection\` is \`RenditionSelection.CUSTOM\`, got ${this.props.renditionSelection}.`);
+    }


This validation can be removed if we use an enum-like class instead of the enum for rendtitionSelection and enforce when renditions needs to be provided in the parameters of which selection is chosen.

Removed validations.

paulhcsun · 2024-11-05T00:40:25Z

packages/@aws-cdk/aws-ivs-alpha/lib/recording-configuration.ts

+   *
+   * @default ThumbnailRecordingMode.INTERVAL
+   */
+  readonly thumbnailRecordingMode?: ThumbnailRecordingMode;


Would also prefer to use an enum-like class here to enforce which properties must be set together and make it impossible to set conflicting properties together. Then we could remove the validateThumbnailSettings method.

I've created ThumbnailConfiguration class.

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

Pull request has been modified.

mazyu36 · 2024-11-05T15:52:52Z

@paulhcsun
Thank you for the review!
I think your suggestion is good, so I've fixed.

paulhcsun · 2024-11-05T23:18:54Z

packages/@aws-cdk/aws-ivs-alpha/lib/recording-configuration.ts

+  /**
+   * A rendition configuration describes which renditions should be recorded for a stream.
+   *
+   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ivs-recordingconfiguration-renditionconfiguration.html
+   *
+   * @default - no rendition configuration
+   */
+  readonly renditionConfiguration?: RenditionConfiguration;
+
+  /**
+   * A thumbnail configuration enables/disables the recording of thumbnails for a live session and controls the interval at which thumbnails are generated for the live session.
+   *
+   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ivs-recordingconfiguration-thumbnailconfiguration.html
+   *
+   * @default - no thumbnail configuration
+   */
+  readonly thumbnailConfiguration?:ThumbnailConfiguration;
+}


Thanks for making the requested changes @mazyu36! I notice now though that the properties that are contained within the renditionConfiguration and thumbnailConfiguration no longer have defaults because the new default is that the configuration is not provided. Is this intentional or should we still set a default for these properties somewhere?

@paulhcsun
Thanks.
The lack of default values is intentional. Is it correct to understand that you're suggesting we should document somewhere what the default values are when @param is omitted? If so, I plan to add this information to the @param description.

I added an explanation regarding the default value. Please let me know if it doesn’t match your intention.

Yes the documentation is appreciated! But also I wanted to check if this default is something that gets set by the service/cloudformation fills it in, since we're not explicitly setting it as the default anywhere.

Thanks.
Those default values are provided by CFn.
So I've not explicitly set.

Gotcha, I'm good to approve then. Thank you for working on this @mazyu36!!

paulhcsun · 2024-11-07T20:35:01Z

@Mergifyio update

mergify · 2024-11-07T20:35:11Z

update

☑️ Nothing to do

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

aws-cdk-automation · 2024-11-07T21:05:24Z

AWS CodeBuild CI Report

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

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

mergify · 2024-11-07T21:05:46Z

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

github-actions · 2024-11-07T21:06:06Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

mazyu36 added 3 commits October 22, 2024 13:14

add recording configuraiton

577179f

add validation

1a0c5f9

add README

1ffa50b

aws-cdk-automation requested a review from a team October 25, 2024 08:45

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Oct 25, 2024

Update README.md

2859000

mazyu36 marked this pull request as ready for review October 25, 2024 12:16

mazyu36 added 2 commits October 25, 2024 21:16

Merge branch 'main' into ivs-recording-configuration-31780

0c4d481

docs update

38ce28e

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 25, 2024

add import method test

4af5a13

mazyu36 force-pushed the ivs-recording-configuration-31780 branch from a201f7f to 4af5a13 Compare October 26, 2024 04:38

github-actions bot mentioned this pull request Oct 28, 2024

Weekly PR metrics report #31915

Closed

paulhcsun self-assigned this Oct 28, 2024

github-actions bot mentioned this pull request Nov 1, 2024

Monthly PR metrics report #31972

Closed

paulhcsun previously requested changes Nov 5, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 5, 2024

Update README.md

4273000

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 5, 2024

mazyu36 added 2 commits November 6, 2024 00:20

incorporate review comments

7e41a7f

Merge branch 'main' into ivs-recording-configuration-31780

3b05eb5

paulhcsun reviewed Nov 5, 2024

View reviewed changes

add description about default value

8fad7d5

mazyu36 requested a review from paulhcsun November 6, 2024 11:05

Merge branch 'main' into ivs-recording-configuration-31780

b323f4d

paulhcsun approved these changes Nov 7, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 7, 2024

mergify bot merged commit 8a3734d into aws:main Nov 7, 2024
16 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2024

mazyu36 deleted the ivs-recording-configuration-31780 branch November 7, 2024 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ivs): support recording configuration for channel #31899

feat(ivs): support recording configuration for channel #31899

mazyu36 commented Oct 25, 2024 •

edited

Loading

paulhcsun left a comment

paulhcsun Nov 5, 2024

mazyu36 Nov 5, 2024

paulhcsun Nov 5, 2024

mazyu36 Nov 5, 2024

paulhcsun Nov 5, 2024

mazyu36 Nov 5, 2024

mazyu36 commented Nov 5, 2024

paulhcsun Nov 5, 2024

mazyu36 Nov 5, 2024 •

edited

Loading

mazyu36 Nov 6, 2024

paulhcsun Nov 6, 2024

mazyu36 Nov 6, 2024

paulhcsun Nov 7, 2024

paulhcsun commented Nov 7, 2024

mergify bot commented Nov 7, 2024

aws-cdk-automation commented Nov 7, 2024

mergify bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

feat(ivs): support recording configuration for channel #31899

feat(ivs): support recording configuration for channel #31899

Conversation

mazyu36 commented Oct 25, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

paulhcsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 commented Nov 5, 2024

Choose a reason for hiding this comment

mazyu36 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulhcsun commented Nov 7, 2024

mergify bot commented Nov 7, 2024

☑️ Nothing to do

aws-cdk-automation commented Nov 7, 2024

AWS CodeBuild CI Report

mergify bot commented Nov 7, 2024

github-actions bot commented Nov 7, 2024

mazyu36 commented Oct 25, 2024 •

edited

Loading

mazyu36 Nov 5, 2024 •

edited

Loading