Skip to content
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

Add support for processing_units to google_spanner_instance #4993

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions mmv1/products/spanner/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,13 @@ objects:
required: true
- !ruby/object:Api::Type::Integer
name: 'nodeCount'
description: 'The number of nodes allocated to this instance.'
default_value: 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by note: While removing a default is a safe change, that's only if the field is moved to Optional + Computed. We may need to use a CustomizeDiff to simulate a default of 1 when processing_units is unset, as breaking a config like the following working would also be backwards-incompatible. (Changing API requirements make backwards compatibility difficult!)

resource "google_spanner_instance" "example" {
  config       = "regional-us-central1"
  display_name = "Test Spanner Instance"
  labels = {
    "foo" = "bar"
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if:

  • I write an encoder that sets num_nodes to 1 if processing_units is 0, mark num_nodes as O+C and remove the default.
  • Open an issue to make both of the fields ExactlyOneOf in v4 and remove the encoder?
  • File an issue to fix that bug in the ruby generator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that should also work. We should make sure to test a config w/o an explicit node_count if we don't already.

description: |
The number of nodes allocated to this instance. At most one of either node_count or processing_units can be present in terraforn.
c2thorn marked this conversation as resolved.
Show resolved Hide resolved
- !ruby/object:Api::Type::Integer
name: 'processingUnits'
description: |
The number of processing units allocated to this instance. At most one of processing_units
or node_count can be present in terraform.
- !ruby/object:Api::Type::KeyValuePairs
name: 'labels'
description: |
Expand Down
8 changes: 8 additions & 0 deletions mmv1/products/spanner/terraform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ overrides: !ruby/object:Overrides::ResourceOverrides
primary_resource_id: "example"
# Randomness
skip_vcr: true
- !ruby/object:Provider::Terraform::Examples
name: "spanner_instance_processing_units"
primary_resource_id: "example"
# Randomness
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the randomness here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. Copied the other block

skip_vcr: true
- !ruby/object:Provider::Terraform::Examples
name: "spanner_instance_multi_regional"
primary_resource_id: "example"
Expand Down Expand Up @@ -108,6 +113,9 @@ overrides: !ruby/object:Overrides::ResourceOverrides
custom_flatten: 'templates/terraform/custom_flatten/name_from_self_link.erb'
nodeCount: !ruby/object:Overrides::Terraform::PropertyOverride
name: num_nodes
default_from_api: true
processingUnits: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
config: !ruby/object:Overrides::Terraform::PropertyOverride
custom_expand: templates/terraform/custom_expand/spanner_instance_config.go.erb
state: !ruby/object:Overrides::Terraform::PropertyOverride
Expand Down
4 changes: 4 additions & 0 deletions mmv1/templates/terraform/encoders/spanner_instance.go.erb
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// Temp Logic to accomodate processing_units and num_nodes
if obj["processingUnits"] == nil && obj["nodeCount"] == nil {
obj["nodeCount"] = 1
}
newObj := make(map[string]interface{})
newObj["instance"] = obj
if obj["name"] == nil {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
resource "google_spanner_instance" "example" {
config = "regional-us-central1"
display_name = "Test Spanner Instance"
processing_units = 200
labels = {
"foo" = "bar"
}
}
34 changes: 34 additions & 0 deletions mmv1/third_party/terraform/tests/resource_spanner_instance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,30 @@ func TestAccSpannerInstance_basic(t *testing.T) {
})
}

func TestAccSpannerInstance_noNodeCountSpecified(t *testing.T) {
t.Parallel()

idName := fmt.Sprintf("spanner-test-%s", randString(t, 10))
vcrTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckSpannerInstanceDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccSpannerInstance_noNodeCountSpecified(idName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrSet("google_spanner_instance.basic", "state"),
),
},
{
ResourceName: "google_spanner_instance.basic",
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccSpannerInstance_basicWithAutogenName(t *testing.T) {
// Randomness
skipIfVcr(t)
Expand Down Expand Up @@ -140,6 +164,16 @@ resource "google_spanner_instance" "basic" {
`, name, name)
}

func testAccSpannerInstance_noNodeCountSpecified(name string) string {
return fmt.Sprintf(`
resource "google_spanner_instance" "basic" {
name = "%s"
config = "regional-us-central1"
display_name = "%s-dname"
}
`, name, name)
}

func testAccSpannerInstance_basicWithAutogenName(name string) string {
return fmt.Sprintf(`
resource "google_spanner_instance" "basic" {
Expand Down