-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 typo in aws_medialive_channel
documentation
#33410
Conversation
Community NoteVoting for Prioritization
For Submitters
|
Looks like the schema should be updated to match docs. https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/medialive/types#AutomaticInputFailoverSettings |
Hey @gbw 👋 Nice catch! I agree with you that updating the schema to more accurately match the SDK is probably the better way to go about this. I think that would be classified as a breaking change, so I'll wait on one of the provider team folks to chime in as well to see how they'd like to go about it. |
Sounds good to me. Just for reference the flatten/expand for this type was only just added in 5.16.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
For arguments which are arrays in the AWS API, but represented as blocks in the AWS Provider, we often adopt singular versions of the attribute name for improved readability. In this instance,
resource "aws_medialive_channel" "example" {
# this would be nested in a real configuration, but for simplicity is represented as a root argument here
failover_condition {
# condition 1
}
failover_condition {
# condition 2
}
}
Reads slightly better than having multiple blocks titled failure_conditions
resource "aws_medialive_channel" "example" {
failover_conditions {
# condition 1
}
failover_conditions {
# condition 2
}
}
In this case my personal opinion is that the documentation update is sufficient.
Please update the other |
Thanks for the context @jar-b. I think this case is a little odd since within a failure conditions block multiple conditions can be set (i.e. audio, input, and/or video). |
I don't think this is a typo in the documentation, or at least not only the documentation. The field doesn't work because half of the code uses the the old value and half uses the new value. We've the opportunity to pick the pluralization that we want here since I don't think this field ever worked, so would not be a breaking change in my book. See my comment on a related issue #33550 (comment) |
@justinretzolk could you add this change or would you prefer a separate pull request to fix? |
Thank you for your contribution! 🚀 A new usage of AWS SDK for Go V1 was detected. Please prefer AWS SDK for Go V2 for all net-new services. If this is an enhancement or bug fix to an existing AWS SDK Go V1 based resource, this comment can be safely ignored. For additional information refer to the AWS SDK for Go Versions page in the contributor guide. |
Hey @gbw @evelynhathaway 👋 Thank you for the additional input here! I've pushed additional changes to address those points as well. |
This functionality has been released in v5.31.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Description
This PR fixes a typo in the
aws_medialive_channel
documentation, so that the docs match the underlying schema.Important
As called out in the comments below, the field in the SDK is actually
FailoverConditions
, as the docs currently list. If I'm not mistaken, making that change would be considered breaking, so I'm interested in input on which way to go with this.Relations
Closes #33407
Closes #33550
References
Schema
Output from Acceptance Testing
N/a, docs