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

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

Merged
merged 12 commits into from
Nov 7, 2024

Conversation

mazyu36
Copy link
Contributor

@mazyu36 mazyu36 commented Oct 25, 2024

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


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

@aws-cdk-automation aws-cdk-automation requested a review from a team October 25, 2024 08:45
@github-actions 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
@mazyu36 mazyu36 marked this pull request as ready for review October 25, 2024 12:16
@aws-cdk-automation 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
@mazyu36 mazyu36 force-pushed the ivs-recording-configuration-31780 branch from a201f7f to 4af5a13 Compare October 26, 2024 04:38
@paulhcsun paulhcsun self-assigned this Oct 28, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Comment on lines 42 to 48
/**
* 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[];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created RenditionConfiguration class.

Comment on lines 322 to 329
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}.`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed validations.

*
* @default ThumbnailRecordingMode.INTERVAL
*/
readonly thumbnailRecordingMode?: ThumbnailRecordingMode;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created ThumbnailConfiguration class.

@aws-cdk-automation 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
Co-authored-by: paulhcsun <[email protected]>
@mergify mergify bot dismissed paulhcsun’s stale review November 5, 2024 13:50

Pull request has been modified.

@aws-cdk-automation 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
Copy link
Contributor Author

mazyu36 commented Nov 5, 2024

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

Comment on lines +35 to +52
/**
* 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mazyu36 mazyu36 Nov 5, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@mazyu36 mazyu36 requested a review from paulhcsun November 6, 2024 11:05
@paulhcsun
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Nov 7, 2024

update

☑️ Nothing to do

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

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

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

Copy link
Contributor

mergify bot commented Nov 7, 2024

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

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

github-actions bot commented Nov 7, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2024
@mazyu36 mazyu36 deleted the ivs-recording-configuration-31780 branch November 7, 2024 23:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ivs: support RecordingConfiguration for channel
3 participants