service/apigateway: Fix schema set errors

[appilon]

Removed undefined and unneeded request_validator_id from request validator import.
Set settings properly and return error, this required making the fields optional + computed. The acceptance tests would have never caught this error since the fields are user configurable, the config would be the defacto state.

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #13312

Release note for CHANGELOG:

* resource/aws_api_gateway_method_settings: `settings` now properly set

Output from acceptance testing:

❯ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayMethodSettings'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSAPIGatewayMethodSettings -timeout 120m
=== RUN   TestAccAWSAPIGatewayMethodSettings_basic
=== PAUSE TestAccAWSAPIGatewayMethodSettings_basic
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_Multiple
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_Multiple
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit
=== RUN   TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy
=== PAUSE TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy
=== CONT  TestAccAWSAPIGatewayMethodSettings_basic
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_Multiple
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl
=== CONT  TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled (54.34s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds (54.57s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit (116.15s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_Multiple (149.83s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit (181.82s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_basic (251.95s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled (265.11s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy (279.81s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted (383.68s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl (476.06s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled (535.43s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel (536.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	539.755s

[anGie44]

LGTM, @appilon 👍

I think we can overlook the failing acceptance test results (in APIGatewayMethodSettings) since they're unrelated?

[appilon]

Thanks for review @anGie44 , are you saying you did run the tests in CI and they fail unrelatedly? I haven't had a chance to setup that tool for kicking off specific tests

[anGie44]

hmm I've looking over the results again and actually I think I may have based that comment on the Test History success rate 😞 ..in master the tests look to be passing and the results for this branch do show diffs in the settings attribute so they must be in some way related 😅 sorry for the confusion!

looking at the basic test, where the config only has the logging_level set in settings, it seems the API returns some default values causing the diff ... interesting, so previously setting a nested attribute like d.Set("settings.0.cache_enabled", ...) didn't actually set the field?

[appilon]

@anGie44 Previously all the settings fields would error, this meant that what was passed in user config would remain in state. This sort of "didn't error check, but all tests still pass" thing happens often as acceptance tests are trying to detect drift between Create and Read or Update and Read, but if Read isn't actually setting anything, then the acceptance test passes since the config becomes the state.

I will look into the failures but I suspect what you claim is right, now we ARE setting values and they don't align with config so we are getting drift.

UPDATE:
The the API response has some setting defaults, marked fields as optional + computed

[anGie44]

Nice, I think semantically, using the Computed attribute makes sense here since the settings are set behind the scenes (and personally I didn't come across docs with defaults specified, though maybe in the console they're visible?) and perhaps future upstream changes will affect the defaults which would be cumbersome to keep up-to-date if the Default attribute were to be used instead.

Output of acceptance tests:

--- PASS: TestAccAWSAPIGatewayMethodSettings_basic (9.91s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CachingEnabled (40.82s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_MetricsEnabled (71.04s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheTtlInSeconds (106.59s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_RequireAuthorizationForCacheControl (141.98s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingBurstLimit (142.27s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_DataTraceEnabled (276.88s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_UnauthorizedCacheControlHeaderStrategy (368.59s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_ThrottlingRateLimit (372.88s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_LoggingLevel (373.72s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_Multiple (555.89s)
--- PASS: TestAccAWSAPIGatewayRequestValidator_basic (600.02s)
--- PASS: TestAccAWSAPIGatewayMethodSettings_Settings_CacheDataEncrypted (670.64s)

[appilon]

Thanks @anGie44 , so this is approved?

[anGie44]

ah let's see, i think another look would be helpful before merging 😅. tagging @gdavison, @bflad for another set of eyes

[bflad]

Using Computed: true for all the nested attributes is the safest option for now. 👍

[ghost]

This has been released in version 2.64.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 for triage. Thanks!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!