feat: implementation of spanner instance_type to be able to provision a FREE_INSTANCE via terraform

[ramonvermeulen]

Solves hashicorp/terraform-provider-google#22596

feat: implementation of spanner instance_type to be able to provision a FREE_INSTANCE via terraform

• had to implement a custom version of exactly_one_of because when provisioning a spanner instance with instance_type = FREE_INSTANCE neither one of num_nodes or processing_units or autoscaling_config is allowed to be configured.

Added / direct affected tests pass locally:

Couldn't run all tests locally with -run=TestAccSpannerInstance_ because I ran into some org / quota policies on my sandbox project.

Another thing to keep in mind, that makes testing a bit harder:

E.g.: tests always have to run on a "new" GCP project, not sure if that is solved within the CI?

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

spanner: added field `instance_type` to the `google_spanner_instance` resource

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: see go/terraform-auto-test-runs to set up automatic test runs.

@slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 4 files changed, 128 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 4 files changed, 128 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 35 insertions(+))

[modular-magician]

Tests analytics

Total tests: 44
Passed tests: 32
Skipped tests: 5
Affected tests: 7

Click here to see the affected service packages

• spanner

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccSpannerInstance_basicWithAsymmetricAutoscalingConfigsUpdate
• TestAccSpannerInstance_basicWithAutoscalingUsingNodeConfigUpdate
• TestAccSpannerInstance_basicWithAutoscalingUsingNodeConfigUpdateDisableAutoscaling
• TestAccSpannerInstance_basicWithAutoscalingUsingPrecessingUnitsConfigUpdateDisableAutoscaling
• TestAccSpannerInstance_basicWithAutoscalingUsingProcessingUnitConfigUpdate
• TestAccSpannerInstance_freeInstanceBasicUpdate
• TestAccSpannerInstance_update

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccSpannerInstance_basicWithAsymmetricAutoscalingConfigsUpdate [Debug log]
TestAccSpannerInstance_basicWithAutoscalingUsingNodeConfigUpdate [Debug log]
TestAccSpannerInstance_basicWithAutoscalingUsingNodeConfigUpdateDisableAutoscaling [Debug log]
TestAccSpannerInstance_basicWithAutoscalingUsingPrecessingUnitsConfigUpdateDisableAutoscaling [Debug log]
TestAccSpannerInstance_basicWithAutoscalingUsingProcessingUnitConfigUpdate [Debug log]
TestAccSpannerInstance_freeInstanceBasicUpdate [Debug log]
TestAccSpannerInstance_update [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🟢 All tests passed!

View the build log or the debug log for each test

[github-actions]

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[slevenick]

I don't think we want to throw this type of error here. exactly_one_of should present a plan-time error, but this is a runtime error.

We may need to remove the exactly_one_of, or replace it with at_least_one_of and include instance_type

[ramonvermeulen]

So the problem I ran into is that exactly_one_of that was already configured for num_nodes, processing_units and autoscaling_config. However, this now conflicts with instance_type == FREE_INSTANCE, e.g. with FREE_INSTANCE none of these fields are allowed to be configured, while with PROVISIONED instance exactly one of these fields should be configured. So my thought was to do a custom implementation in the encoder to solve this issue.

What would be your suggestion to solve this problem without causing a runtime error.

[slevenick]

I would probably move to at_least_one_of on all 4 of the fields. It won't be the most restrictive that we could be, but it will accomplish most of the goals at plan time. Most users will continue not setting instance_type, and instead have to specify one of the other fields

[ramonvermeulen]

I see, that makes sense. With the idea of it being an edge case, and the API will reject it with a descriptive message during the apply phase anyways for the users that still configure both?

[ramonvermeulen]

Just applied this, but thinking about it now this doesn't solve the problem (test TestAccSpannerInstance_freeInstanceBasicUpdate also fails now). Because at_least_one_of requires that either num_nodes, processing_units or autoscaling_config is set. For a FREE_INSTANCE none of these fields can be set which conflicts with "at least one of ...".

I guess the best way moving forward is only setting conflicts?

[slevenick]

I would add instance_type to the at_least_one_of. So a user can set instance_type = FREE instead of setting one of the other values.

Most people won't specify instance_type = PROVISIONED as examples won't specify, so the at_least_one_of will mostly work as intended

[ramonvermeulen]

Hmm I see, only weird scenario is then indeed when a user configures only instance_type=PROVISIONED (although not in any of the examples) on the api endpoint (during apply) you will probably still get an error even though the plan succeeds. But that is in this case the trade-off I guess.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 4 files changed, 128 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 4 files changed, 128 insertions(+), 4 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 35 insertions(+))

[modular-magician]

Tests analytics

Total tests: 45
Passed tests: 39
Skipped tests: 5
Affected tests: 1

Click here to see the affected service packages

• spanner

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccSpannerInstance_freeInstanceBasicUpdate

Get to know how VCR tests work

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccSpannerInstance_freeInstanceBasicUpdate [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[github-actions]

@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[ramonvermeulen]

Couldn't run all tests (some spanner tests are quite expensive, at least I saw it billed €60~ on my companies cloud sandbox where I once ran all tests, don't want to run that on my private GCP org).
But at least these two succeed now:

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 4 files changed, 130 insertions(+), 15 deletions(-))
google-beta provider: Diff ( 4 files changed, 130 insertions(+), 15 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 21 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 45
Passed tests: 40
Skipped tests: 5
Affected tests: 0

Click here to see the affected service packages

• spanner

🟢 All tests passed!

View the build log

[slevenick]

Thanks!