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 accelerators to google_datafusion_instance #6851

Merged
merged 13 commits into from
Mar 8, 2023
24 changes: 24 additions & 0 deletions mmv1/products/datafusion/Instance.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -227,4 +227,28 @@ properties:
The resource name of the Pub/Sub topic. Format: projects/{projectId}/topics/{topic_id}
required: true
input: true
- !ruby/object:Api::Type::Array
name: 'accelerators'
description: |
List of accelerators enabled for this CDF instance.

If accelerators are enabled it is possible a permadiff will be created with the Options field.
Users will need to either manually update their state file to include these diffed options, or include the field in a [lifecycle ignore changes block](https://developer.hashicorp.com/terraform/language/meta-arguments/lifecycle#ignore_changes).
item_type: !ruby/object:Api::Type::NestedObject
properties:
- !ruby/object:Api::Type::Enum
name: 'acceleratorType'
description: |
The type of an accelator for a CDF instance.
values:
- :CDC
- :HEALTHCARE
- :CCAI_INSIGHTS
required: true
- !ruby/object:Api::Type::Enum
name: 'state'
Copy link
Member

@rileykarson rileykarson Mar 9, 2023

Choose a reason for hiding this comment

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

drive-by post review comment: We could consider marking this as required: true as well, or mark a clientside "ENABLED" default. We need more complete guidance on this, but we want to avoid default_from_api wherever possible- it creates worse Terraform plans, and can have surprising behaviour when performing complex updates. We still need to use it a lot of the time, though, particularly on already-released fields.

Particularly, there are often cases where making a field (in a new resource) or a subfield (of a new field) required is preferable to mapping to the minimum possible message the API allows. It creates a very clear schema, and makes the user's configuration+plan very explicit about what's happening. Plus, it's possible to change the schema in a minor version- including adding a clientside default. That's not an option for new top-level fields in existing resources or subfields in existing fields, since that'd be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a general rule for when to approach something with "this supports the main use cases better" as opposed to just matching the API functionality? Or is it just the kind of thing that will make more sense with further experience working with the provider and adding more resources and functionality?

Copy link
Member

Choose a reason for hiding this comment

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

Right now just further experience- we need to create guidance/define a general approach, but the rules will always be pretty subjective.

My thinking here is: consider the potential for confusion here if a user has a DISABLED accelerator and the state field is not in config Terraform won't see a diff, and recreating will create a resource in a different state- vs the annoyance of typing (and realistically, copy-pasting) "ENABLED" into configs.

description: |
The type of an accelator for a CDF instance.
values:
- :ENABLED
- :DISABLED
8 changes: 7 additions & 1 deletion mmv1/products/datafusion/terraform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
instance_name: "my-instance"
custom_code: !ruby/object:Provider::Terraform::CustomCode
pre_update: templates/terraform/pre_update/datafusion_instance_update.go.erb
constants: templates/terraform/constants/data_fusion_instance_option.go.erb
properties:
region: !ruby/object:Overrides::Terraform::PropertyOverride
ignore_read: true
Expand All @@ -77,8 +78,13 @@ overrides: !ruby/object:Overrides::ResourceOverrides
name: !ruby/object:Overrides::Terraform::PropertyOverride
custom_expand: 'templates/terraform/custom_expand/shortname_to_url.go.erb'
custom_flatten: 'templates/terraform/custom_flatten/name_from_self_link.erb'
zone: !ruby/object:Overrides::Terraform::PropertyOverride
options: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
diff_suppress_func: 'instanceOptionsDiffSuppress'
zone: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
accelerators.state: !ruby/object:Overrides::Terraform::PropertyOverride
default_from_api: true
# This is for copying files over
files: !ruby/object:Provider::Config::Files
# These files have templating (ERB) code that will be run.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
var instanceAcceleratorOptions = []string{
"delta.default.checkpoint.directory",
"ui.feature.cdc",
}

func instanceOptionsDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
// Suppress diffs for the options generated by adding an accelerator to a data fusion instance
for _, option := range instanceAcceleratorOptions {
if strings.Contains(k, option) && new == "" {
return true
}
}

// Let diff be determined by options (above)
if strings.Contains(k, "options.%") {
return true
}

// For other keys, don't suppress diff.
return false
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ resource "google_data_fusion_instance" "<%= ctx[:primary_resource_id] %>" {
ip_allocation = "${google_compute_global_address.private_ip_alloc.address}/${google_compute_global_address.private_ip_alloc.prefix_length}"
}

accelerators {
accelerator_type = "CDC"
state = "ENABLED"
}
<%= ctx[:vars]['prober_test_run'] %>
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ resource "google_data_fusion_instance" "foobar" {
options = {
prober_test_run = "true"
}
accelerators {
accelerator_type = "CDC"
state = "DISABLED"
}
}
`, instanceName)
}
Expand All @@ -66,6 +70,11 @@ resource "google_data_fusion_instance" "foobar" {
label2 = "value2"
}
version = "6.8.0"

accelerators {
accelerator_type = "CCAI_INSIGHTS"
state = "ENABLED"
}
# Mark for testing to avoid service networking connection usage that is not cleaned up
options = {
prober_test_run = "true"
Expand Down