feat: Add video_data_delivery_enabled arg for aws_bedrock_model_invocation_logging_configuration #41317

acwwat · 2025-02-11T05:02:05Z

Description

This PR is to add the video_data_delivery_enabled argument to the aws_bedrock_model_invocation_logging_configuration resource. The *_data_delivery_enabled arguments were set to required when in fact they are not, so I've changed them to optional with a default value of true to match the API behavior observed from testing it.

Updated 2025-02-12: I've also converted all SingleNestedBlocks to ListNestedBlocks to fix validation issues related to #35813.

Relations

Closes #41312
Relates #35813

References

Referred to PutModelInvocationLoggingConfiguration for specs and wordings.

Output from Acceptance Testing

$ make testacc TESTS=TestAccBedrock_serial/ModelInvocationLoggingConfiguration PKG=bedrock
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/bedrock/... -v -count 1 -parallel 20 -run='TestAccBedrock_serial/ModelInvocationLoggingConfiguration'  -timeout 360m -vet=off
2025/02/12 22:19:57 Initializing Terraform AWS Provider...
=== RUN   TestAccBedrock_serial
=== PAUSE TestAccBedrock_serial
=== CONT  TestAccBedrock_serial
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration/basic
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration/disappears
--- PASS: TestAccBedrock_serial (77.69s)
    --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration (77.69s)
        --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration/basic (49.51s)
        --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration/disappears (28.18s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/bedrock    77.926s

$

github-actions · 2025-02-11T05:02:21Z

Community Note

Voting for Prioritization

Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
Please see our prioritization guide for information on how we prioritize.
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

acwwat · 2025-02-12T22:05:07Z

Just noticed that there is already #35813 opened for an unrelated issue I noticed while I was working on this PR. I will look into fixing that issue separately from this PR.

…ation_logging_configuration

…required states for aws_bedrock_model_invocation_logging_configuration

.changelog/41317.txt

jar-b · 2025-02-14T16:28:18Z

Confirmed with the AWS API that all arguments converted to required are in fact marked required in the API documentation as well. As such, this technically breaking change is determined to be acceptable within a minor version.

jar-b

LGTM 🎉

% make testacc PKG=bedrock TESTS=TestAccBedrock_serial/ModelInvocationLoggingConfiguration
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/bedrock/... -v -count 1 -parallel 20 -run='TestAccBedrock_serial/ModelInvocationLoggingConfiguration'  -timeout 360m -vet=off
2025/02/14 11:26:26 Initializing Terraform AWS Provider...
=== RUN   TestAccBedrock_serial
=== PAUSE TestAccBedrock_serial
=== CONT  TestAccBedrock_serial
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration/basic
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration/disappears
--- PASS: TestAccBedrock_serial (63.98s)
    --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration (63.98s)
        --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration/basic (42.36s)
        --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration/disappears (21.63s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/bedrock    70.815s

jar-b · 2025-02-14T16:29:07Z

Thanks for your contribution, @acwwat! 👍

jar-b · 2025-02-14T19:10:39Z

Due to how migration from SingleNestedBlock to ListNestedBlock changes the underlying resource schema, a state upgrader will need to be included here, along with an acceptance test to verify the upgrade. I'm also going to add this PR and the others migrating SingleNestedBlock types to the v6.0.0 so that we minimize any potential disruption in the behavior of impacted resources across minor versions.

Thanks again for your effort on cleaning these up @acwwat, it is much appreciated!

jar-b · 2025-02-14T19:12:10Z

Alternatively, if you'd like to remove the SingleNestedBlock conversion and leave the other remaining changes we can merge this now and save that piece for V6 later.

This reverts commit a18cfdc.

…ted arg required states for aws_bedrock_model_invocation_logging_configuration" This reverts commit 1ee270d.

acwwat · 2025-02-14T19:48:27Z

Alternatively, if you'd like to remove the SingleNestedBlock conversion and leave the other remaining changes we can merge this now and save that piece for V6 later.

@jar-b Thanks for your detailed review of this PR and the other ones related to SingleNestedBlocks. I've opted to revert the SingleNestedBlock change so that we can first get the missing argument in. Appreciate if you could re-review this PR once all the checks pass.

jar-b · 2025-02-14T21:31:29Z

Sounds good - thanks for the flexibility!

jar-b

LGTM 🎉

% make testacc PKG=bedrock TESTS=TestAccBedrock_serial/ModelInvocationLoggingConfiguration
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/bedrock/... -v -count 1 -parallel 20 -run='TestAccBedrock_serial/ModelInvocationLoggingConfiguration'  -timeout 360m -vet=off
2025/02/14 16:36:51 Initializing Terraform AWS Provider...
=== RUN   TestAccBedrock_serial
=== PAUSE TestAccBedrock_serial
=== CONT  TestAccBedrock_serial
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration/basic
=== RUN   TestAccBedrock_serial/ModelInvocationLoggingConfiguration/disappears
--- PASS: TestAccBedrock_serial (66.67s)
    --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration (66.67s)
        --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration/basic (41.19s)
        --- PASS: TestAccBedrock_serial/ModelInvocationLoggingConfiguration/disappears (25.48s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/bedrock    73.279s

acwwat requested a review from a team as a code owner February 11, 2025 05:02

acwwat force-pushed the f-aws_bedrock_model_invocation_logging_configuration-add_video_data_delivery_enabled_arg branch 4 times, most recently from abe7ee0 to 30d433e Compare February 11, 2025 05:17

jar-b removed the needs-triage Waiting for first response or review from a maintainer. label Feb 11, 2025

justinretzolk added bug Addresses a defect in current functionality. enhancement Requests to existing resources that expand the functionality or scope. labels Feb 11, 2025

feat: Add video_data_delivery_enabled arg for aws_bedrock_model_invoc…

fe15338

…ation_logging_configuration

acwwat force-pushed the f-aws_bedrock_model_invocation_logging_configuration-add_video_data_delivery_enabled_arg branch from 30d433e to ea76035 Compare February 13, 2025 03:22

acwwat mentioned this pull request Feb 13, 2025

Disallow use of SingleNestedBlock in resource schemas #35813

Open

fix: Convert SingleNestedBlock to ListNestedBlock and fix nested arg …

1ee270d

…required states for aws_bedrock_model_invocation_logging_configuration

acwwat force-pushed the f-aws_bedrock_model_invocation_logging_configuration-add_video_data_delivery_enabled_arg branch from ea76035 to 1ee270d Compare February 13, 2025 03:44

jar-b reviewed Feb 14, 2025

View reviewed changes

.changelog/41317.txt Outdated Show resolved Hide resolved

chore: adjust changelog

a18cfdc

jar-b previously approved these changes Feb 14, 2025

View reviewed changes

jar-b added this to the v6.0.0 milestone Feb 14, 2025

acwwat added 2 commits February 14, 2025 14:43

Revert "chore: adjust changelog"

9f73e68

This reverts commit a18cfdc.

Revert "fix: Convert SingleNestedBlock to ListNestedBlock and fix nes…

80215af

…ted arg required states for aws_bedrock_model_invocation_logging_configuration" This reverts commit 1ee270d.

acwwat dismissed jar-b’s stale review via 80215af February 14, 2025 19:44

jar-b removed this from the v6.0.0 milestone Feb 14, 2025

jar-b approved these changes Feb 14, 2025

View reviewed changes

jar-b merged commit 0017304 into hashicorp:main Feb 18, 2025
43 checks passed

github-actions bot added this to the v5.88.0 milestone Feb 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add video_data_delivery_enabled arg for aws_bedrock_model_invocation_logging_configuration #41317

feat: Add video_data_delivery_enabled arg for aws_bedrock_model_invocation_logging_configuration #41317

acwwat commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025

acwwat commented Feb 12, 2025

jar-b commented Feb 14, 2025

jar-b left a comment

jar-b commented Feb 14, 2025

jar-b commented Feb 14, 2025

jar-b commented Feb 14, 2025

acwwat commented Feb 14, 2025 •

edited

Loading

jar-b commented Feb 14, 2025

jar-b left a comment

feat: Add video_data_delivery_enabled arg for aws_bedrock_model_invocation_logging_configuration #41317

feat: Add video_data_delivery_enabled arg for aws_bedrock_model_invocation_logging_configuration #41317

Conversation

acwwat commented Feb 11, 2025 • edited Loading

Description

Relations

References

Output from Acceptance Testing

github-actions bot commented Feb 11, 2025

Community Note

acwwat commented Feb 12, 2025

jar-b commented Feb 14, 2025

jar-b left a comment

Choose a reason for hiding this comment

jar-b commented Feb 14, 2025

jar-b commented Feb 14, 2025

jar-b commented Feb 14, 2025

acwwat commented Feb 14, 2025 • edited Loading

jar-b commented Feb 14, 2025

jar-b left a comment

Choose a reason for hiding this comment

acwwat commented Feb 11, 2025 •

edited

Loading

acwwat commented Feb 14, 2025 •

edited

Loading