feat(ivs): support recording configuration for channel#31899
feat(ivs): support recording configuration for channel#31899mergify[bot] merged 12 commits intoaws:mainfrom
Conversation
a201f7f to
4af5a13
Compare
| /** | ||
| * 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[]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've created RenditionConfiguration class.
| 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}.`); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed validations.
| * | ||
| * @default ThumbnailRecordingMode.INTERVAL | ||
| */ | ||
| readonly thumbnailRecordingMode?: ThumbnailRecordingMode; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've created ThumbnailConfiguration class.
Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>
Pull request has been modified.
|
@paulhcsun |
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I added an explanation regarding the default value. Please let me know if it doesn’t match your intention.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks.
Those default values are provided by CFn.
So I've not explicitly set.
There was a problem hiding this comment.
Gotcha, I'm good to approve then. Thank you for working on this @mazyu36!!
|
@Mergifyio update |
☑️ Nothing to doDetails
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #31780.
Reason for this change
To use recording configuration for IVS channel.
Description of changes
RecordingConfigurationConstruct.recordingConfigurationproperty 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