From caa3f85e85c755d3e2babe761bb2a59c392981c7 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 2 Jun 2021 16:31:33 -0700 Subject: [PATCH 1/9] Allow redisVersion to change, promoted some fields to GA --- mmv1/products/redis/api.yaml | 3 --- ...nce_test.go.erb => resource_redis_instance_test.go} | 10 ++++------ 2 files changed, 4 insertions(+), 9 deletions(-) rename mmv1/third_party/terraform/tests/{resource_redis_instance_test.go.erb => resource_redis_instance_test.go} (96%) diff --git a/mmv1/products/redis/api.yaml b/mmv1/products/redis/api.yaml index 76e86c314076..ecf4bb257a3b 100644 --- a/mmv1/products/redis/api.yaml +++ b/mmv1/products/redis/api.yaml @@ -168,7 +168,6 @@ objects: - REDIS_5_0 for Redis 5.0 compatibility - REDIS_4_0 for Redis 4.0 compatibility - REDIS_3_2 for Redis 3.2 compatibility - input: true - !ruby/object:Api::Type::String name: reservedIpRange description: | @@ -192,7 +191,6 @@ objects: input: true - !ruby/object:Api::Type::Enum name: transitEncryptionMode - min_version: beta input: true description: | The TLS mode of the Redis instance, If not provided, TLS is disabled for the instance. @@ -204,7 +202,6 @@ objects: default_value: :DISABLED - !ruby/object:Api::Type::Array name: 'serverCaCerts' - min_version: beta description: | List of server CA certificates for the instance. output: true diff --git a/mmv1/third_party/terraform/tests/resource_redis_instance_test.go.erb b/mmv1/third_party/terraform/tests/resource_redis_instance_test.go similarity index 96% rename from mmv1/third_party/terraform/tests/resource_redis_instance_test.go.erb rename to mmv1/third_party/terraform/tests/resource_redis_instance_test.go index 1837055c7aef..09ffa0615da8 100644 --- a/mmv1/third_party/terraform/tests/resource_redis_instance_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_redis_instance_test.go @@ -1,5 +1,3 @@ -<% autogen_exception -%> - package google import ( @@ -79,8 +77,8 @@ func TestAccRedisInstance_redisInstanceAuthEnabled(t *testing.T) { } vcrTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - Providers: testAccProviders, + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, CheckDestroy: testAccCheckRedisInstanceDestroyProducer(t), Steps: []resource.TestStep{ { @@ -122,6 +120,7 @@ resource "google_redis_instance" "test" { maxmemory-policy = "allkeys-lru" notify-keyspace-events = "KEA" } + redis_version = "REDIS_4_0" } `, name) } @@ -142,9 +141,8 @@ resource "google_redis_instance" "test" { maxmemory-policy = "noeviction" notify-keyspace-events = "" } - <% unless version == 'ga' -%> transit_encryption_mode = "SERVER_AUTHENTICATION" - <% end -%> + redis_version = "REDIS_5_0" } `, name) } From 192a382ea1dcb701e656055b0eab82f432e27d82 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 9 Jun 2021 13:53:00 -0700 Subject: [PATCH 2/9] allow updating properties if update method is defined at the field level --- mmv1/api/type.rb | 2 +- mmv1/products/redis/terraform.yaml | 2 + mmv1/provider/core.rb | 10 ++ mmv1/templates/terraform/resource.erb | 203 +++++++++++++------------- 4 files changed, 115 insertions(+), 102 deletions(-) diff --git a/mmv1/api/type.rb b/mmv1/api/type.rb index 2e4ef7f05002..2a2d19c47cd2 100644 --- a/mmv1/api/type.rb +++ b/mmv1/api/type.rb @@ -124,7 +124,7 @@ def validate raise 'Property cannot be output and required at the same time.' \ if @output && @required - check :update_verb, type: Symbol, allowed: %i[POST PUT PATCH NONE], + check :update_verb, type: Symbol, allowed: %i[POST PUT PATCH UPGRADE NONE], default: @__resource&.update_verb check :update_url, type: ::String diff --git a/mmv1/products/redis/terraform.yaml b/mmv1/products/redis/terraform.yaml index 1ac586d91e8e..ff0c1a03e453 100644 --- a/mmv1/products/redis/terraform.yaml +++ b/mmv1/products/redis/terraform.yaml @@ -67,6 +67,8 @@ overrides: !ruby/object:Overrides::ResourceOverrides regex: '^[a-z][a-z0-9-]{0,39}[a-z0-9]$' redisVersion: !ruby/object:Overrides::Terraform::PropertyOverride default_from_api: true + update_url: 'projects/{{project}}/locations/{{region}}/instances/{{name}}' + update_verb: :UPGRADE region: !ruby/object:Overrides::Terraform::PropertyOverride ignore_read: true required: false diff --git a/mmv1/provider/core.rb b/mmv1/provider/core.rb index 9809c6aa75c7..fb38eeec9341 100644 --- a/mmv1/provider/core.rb +++ b/mmv1/provider/core.rb @@ -325,6 +325,16 @@ def properties_by_custom_update(properties, behavior = :new) end end + # Filter the properties to keep only the ones don't have custom update + # method and group them by update url & verb. + def properties_without_custom_update(properties) + update_props = properties.reject do |p| + !(p.update_url.nil? || p.update_verb.nil? || p.update_verb == :NOOP) + end + + update_props + end + # Takes a update_url and returns the list of custom updatable properties # that can be updated at that URL. This allows flattened objects # to determine which parent property in the API should be updated with diff --git a/mmv1/templates/terraform/resource.erb b/mmv1/templates/terraform/resource.erb index a83d3068bd50..075befca63f4 100644 --- a/mmv1/templates/terraform/resource.erb +++ b/mmv1/templates/terraform/resource.erb @@ -35,8 +35,8 @@ import ( <% resource_name = product_ns + object.name properties = object.all_user_properties - update_body_properties = object.settable_properties - update_body_properties = object.settable_properties.reject(&:input) if object.update_verb == :PATCH + update_body_properties = properties_without_custom_update(object.settable_properties) + update_body_properties = update_body_properties.reject(&:input) if object.update_verb == :PATCH # Handwritten TF Operation objects will be shaped like accessContextManager while the Google Go Client will have a name like accesscontextmanager client_name = @config.client_name || product_ns client_name_camel = client_name.camelize(:lower) @@ -577,9 +577,105 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ billingProject = project <% end -%> -<% if object.input -%> - d.Partial(true) +<% if !object.input -%> + obj := make(map[string]interface{}) +<% update_body_properties.each do |prop| -%> + <%# flattened objects won't have something stored in state so instead nil is passed to the next expander. -%> + <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%> + <%= prop.api_name -%>Prop, err := expand<%= "Nested" if object.nested_query -%><%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config) + if err != nil { + return err +<% if prop.send_empty_value -%> + } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop) { +<% elsif prop.flatten_object -%> + } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) { +<% else -%> + } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); !isEmptyValue(reflect.ValueOf(v)) && (ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop)) { +<% end -%> + obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop + } +<% end -%> + +<%# We need to decide what encoder to use here - if there's an update encoder, use that! -%> +<% if object.custom_code.update_encoder -%> + obj, err = resource<%= resource_name -%>UpdateEncoder(d, meta, obj) + if err != nil { + return err + } +<% elsif object.custom_code.encoder -%> + obj, err = resource<%= resource_name -%>Encoder(d, meta, obj) + if err != nil { + return err + } +<% end -%> + +<% if object.mutex -%> + lockName, err := replaceVars(d, config, "<%= object.mutex -%>") + if err != nil { + return err + } + mutexKV.Lock(lockName) + defer mutexKV.Unlock(lockName) +<% end -%> + + url, err := replaceVars(d, config, "<%= "{{#{object.__product.name}BasePath}}#{update_uri(object, object.update_url)}" -%>") + if err != nil { + return err + } + + log.Printf("[DEBUG] Updating <%= object.name -%> %q: %#v", d.Id(), obj) +<%= lines(compile(pwd + '/templates/terraform/update_mask.erb')) if object.update_mask -%> +<%= lines(compile(pwd + '/' + object.custom_code.pre_update)) if object.custom_code.pre_update -%> +<% if object.nested_query&.modify_by_patch -%> +<%# Keep this after mutex - patch request data relies on current resource state %> + obj, err = resource<%= resource_name -%>PatchUpdateEncoder(d, meta, obj) + if err != nil { + return err + } +<% end -%> +<% if object.supports_indirect_user_project_override -%> + if parts := regexp.MustCompile(`projects\/([^\/]+)\/`).FindStringSubmatch(url); parts != nil { + billingProject = parts[1] + } +<% end -%> + + // err == nil indicates that the billing_project value was found + if bp, err := getBillingProject(d, config); err == nil { + billingProject = bp + } + + res, err := sendRequestWithTimeout(config, "<%= object.update_verb -%>", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutUpdate)<%= object.error_retry_predicates ? ", " + object.error_retry_predicates.join(',') : "" -%>) + + if err != nil { + return fmt.Errorf("Error updating <%= object.name -%> %q: %s", d.Id(), err) + } else { + log.Printf("[DEBUG] Finished updating <%= object.name -%> %q: %#v", d.Id(), res) + } + +<% if object.async&.allow?('update') -%> +<% if object.async.is_a? Api::OpAsync -%> + err = <%= client_name_camel -%>OperationWaitTime( + config, res, <% if has_project -%> project, <% end -%> "Updating <%= object.name -%>", userAgent, + d.Timeout(schema.TimeoutUpdate)) + + if err != nil { + return err + } +<% elsif object.async.is_a? Provider::Terraform::PollAsync -%> + err = PollingWaitTime(resource<%= resource_name -%>PollRead(d, meta), <%= object.async.check_response_func_existence -%>, "Updating <%= object.name -%>", d.Timeout(schema.TimeoutUpdate), <%= object.async.target_occurrences -%>) + if err != nil { +<% if object.async.suppress_error-%> + log.Printf("[ERROR] Unable to confirm eventually consistent <%= object.name -%> %q finished updating: %q", d.Id(), err) +<% else -%> + return err +<% end -%> + } +<% end -%> +<% end -%> +<% end # if !object.input -%> +<% if properties_by_custom_update(object.root_properties).length() > 0 -%> + d.Partial(true) <% properties_by_custom_update(object.root_properties) .sort_by {|k, _| k.nil? ? "" : k[:update_id].to_s} .each do |key, props| @@ -707,103 +803,8 @@ if <%= props.map { |prop| "d.HasChange(\"#{prop.name.underscore}\")" }.join ' || } <% end -%> - d.Partial(false) -<% else # if object.input -%> - obj := make(map[string]interface{}) -<% update_body_properties.each do |prop| -%> - <%# flattened objects won't have something stored in state so instead nil is passed to the next expander. -%> - <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%> - <%= prop.api_name -%>Prop, err := expand<%= "Nested" if object.nested_query -%><%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config) - if err != nil { - return err -<% if prop.send_empty_value -%> - } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop) { -<% elsif prop.flatten_object -%> - } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) { -<% else -%> - } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); !isEmptyValue(reflect.ValueOf(v)) && (ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop)) { -<% end -%> - obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop - } -<% end -%> - -<%# We need to decide what encoder to use here - if there's an update encoder, use that! -%> -<% if object.custom_code.update_encoder -%> - obj, err = resource<%= resource_name -%>UpdateEncoder(d, meta, obj) - if err != nil { - return err - } -<% elsif object.custom_code.encoder -%> - obj, err = resource<%= resource_name -%>Encoder(d, meta, obj) - if err != nil { - return err - } -<% end -%> - -<% if object.mutex -%> - lockName, err := replaceVars(d, config, "<%= object.mutex -%>") - if err != nil { - return err - } - mutexKV.Lock(lockName) - defer mutexKV.Unlock(lockName) -<% end -%> - - url, err := replaceVars(d, config, "<%= "{{#{object.__product.name}BasePath}}#{update_uri(object, object.update_url)}" -%>") - if err != nil { - return err - } - - log.Printf("[DEBUG] Updating <%= object.name -%> %q: %#v", d.Id(), obj) -<%= lines(compile(pwd + '/templates/terraform/update_mask.erb')) if object.update_mask -%> -<%= lines(compile(pwd + '/' + object.custom_code.pre_update)) if object.custom_code.pre_update -%> -<% if object.nested_query&.modify_by_patch -%> -<%# Keep this after mutex - patch request data relies on current resource state %> - obj, err = resource<%= resource_name -%>PatchUpdateEncoder(d, meta, obj) - if err != nil { - return err - } -<% end -%> -<% if object.supports_indirect_user_project_override -%> - if parts := regexp.MustCompile(`projects\/([^\/]+)\/`).FindStringSubmatch(url); parts != nil { - billingProject = parts[1] - } -<% end -%> - - // err == nil indicates that the billing_project value was found - if bp, err := getBillingProject(d, config); err == nil { - billingProject = bp - } - - res, err := sendRequestWithTimeout(config, "<%= object.update_verb -%>", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutUpdate)<%= object.error_retry_predicates ? ", " + object.error_retry_predicates.join(',') : "" -%>) - - if err != nil { - return fmt.Errorf("Error updating <%= object.name -%> %q: %s", d.Id(), err) - } else { - log.Printf("[DEBUG] Finished updating <%= object.name -%> %q: %#v", d.Id(), res) - } - -<% if object.async&.allow?('update') -%> -<% if object.async.is_a? Api::OpAsync -%> - err = <%= client_name_camel -%>OperationWaitTime( - config, res, <% if has_project -%> project, <% end -%> "Updating <%= object.name -%>", userAgent, - d.Timeout(schema.TimeoutUpdate)) - - if err != nil { - return err - } -<% elsif object.async.is_a? Provider::Terraform::PollAsync -%> - err = PollingWaitTime(resource<%= resource_name -%>PollRead(d, meta), <%= object.async.check_response_func_existence -%>, "Updating <%= object.name -%>", d.Timeout(schema.TimeoutUpdate), <%= object.async.target_occurrences -%>) - if err != nil { -<% if object.async.suppress_error-%> - log.Printf("[ERROR] Unable to confirm eventually consistent <%= object.name -%> %q finished updating: %q", d.Id(), err) -<% else -%> - return err -<% end -%> - } -<% end -%> -<% end -%> -<% end # if object.input -%> + d.Partial(false) +<% end -%> <%= lines(compile(pwd + '/' + object.custom_code.post_update)) if object.custom_code.post_update -%> return resource<%= resource_name -%>Read(d, meta) From edfdcbf1b363d48b299f4f27a13f146d338f8048 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 9 Jun 2021 14:00:25 -0700 Subject: [PATCH 3/9] upgrade post url --- mmv1/api/type.rb | 2 +- mmv1/products/redis/terraform.yaml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mmv1/api/type.rb b/mmv1/api/type.rb index 2a2d19c47cd2..2e4ef7f05002 100644 --- a/mmv1/api/type.rb +++ b/mmv1/api/type.rb @@ -124,7 +124,7 @@ def validate raise 'Property cannot be output and required at the same time.' \ if @output && @required - check :update_verb, type: Symbol, allowed: %i[POST PUT PATCH UPGRADE NONE], + check :update_verb, type: Symbol, allowed: %i[POST PUT PATCH NONE], default: @__resource&.update_verb check :update_url, type: ::String diff --git a/mmv1/products/redis/terraform.yaml b/mmv1/products/redis/terraform.yaml index ff0c1a03e453..b117bcb54498 100644 --- a/mmv1/products/redis/terraform.yaml +++ b/mmv1/products/redis/terraform.yaml @@ -67,8 +67,8 @@ overrides: !ruby/object:Overrides::ResourceOverrides regex: '^[a-z][a-z0-9-]{0,39}[a-z0-9]$' redisVersion: !ruby/object:Overrides::Terraform::PropertyOverride default_from_api: true - update_url: 'projects/{{project}}/locations/{{region}}/instances/{{name}}' - update_verb: :UPGRADE + update_url: 'projects/{{project}}/locations/{{region}}/instances/{{name}}:upgrade' + update_verb: :POST region: !ruby/object:Overrides::Terraform::PropertyOverride ignore_read: true required: false From ac8826b1a4ed6c04ec2a8907422156655d94adc0 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 9 Jun 2021 14:44:07 -0700 Subject: [PATCH 4/9] fix current resources to not reference url --- mmv1/products/compute/api.yaml | 8 -------- mmv1/products/healthcare/api.yaml | 11 ----------- mmv1/templates/terraform/resource.erb | 1 + 3 files changed, 1 insertion(+), 19 deletions(-) diff --git a/mmv1/products/compute/api.yaml b/mmv1/products/compute/api.yaml index b4e940283c78..23210854ed96 100644 --- a/mmv1/products/compute/api.yaml +++ b/mmv1/products/compute/api.yaml @@ -1693,9 +1693,6 @@ objects: # TODO: make a ResourceRef to Security Policy - !ruby/object:Api::Type::String name: 'securityPolicy' - update_verb: :POST - update_url: - 'projects/{{project}}/global/backendServices/{{name}}/setSecurityPolicy' description: | The security policy associated with this backend service. - !ruby/object:Api::Type::Enum @@ -9208,8 +9205,6 @@ objects: - !ruby/object:Api::Type::NestedObject name: 'preservedState' description: 'The preserved state for this instance.' - update_verb: :POST - update_url: 'projects/{{project}}/zones/{{zone}}/instanceGroupManagers/{{instance_group_manager}}/updatePerInstanceConfigs' properties: - !ruby/object:Api::Type::KeyValuePairs name: 'metadata' @@ -9329,7 +9324,6 @@ objects: name: 'preservedState' description: 'The preserved state for this instance.' update_verb: :POST - update_url: 'projects/{{project}}/regions/{{region}}/instanceGroupManagers/{{region_instance_group_manager}}/updatePerInstanceConfigs' properties: - !ruby/object:Api::Type::KeyValuePairs name: 'metadata' @@ -18277,8 +18271,6 @@ objects: the BackendService. The protocol field in the BackendService must be set to GRPC. input: true - update_verb: :PATCH - update_url: projects/{{project}}/global/targetGrpcProxies update_id: 'urlMap' fingerprint_name: 'fingerprint' - !ruby/object:Api::Type::Boolean diff --git a/mmv1/products/healthcare/api.yaml b/mmv1/products/healthcare/api.yaml index 628118e441f5..7b63d3275077 100644 --- a/mmv1/products/healthcare/api.yaml +++ b/mmv1/products/healthcare/api.yaml @@ -54,7 +54,6 @@ objects: input: true - !ruby/object:Api::Type::String name: "timeZone" - update_url: 'projects/{{project}}/locations/{{location}}/datasets/{{name}}' description: | The default timezone used by this dataset. Must be a either a valid IANA time zone name such as "America/New_York" or empty, which defaults to UTC. This is used for parsing times in resources @@ -110,7 +109,6 @@ objects: - !ruby/object:Api::Type::KeyValuePairs name: labels required: false - update_url: '{{dataset}}/dicomStores/{{name}}' description: | User-supplied key-value pairs used to organize DICOM stores. @@ -128,7 +126,6 @@ objects: - !ruby/object:Api::Type::NestedObject name: notificationConfig required: false - update_url: '{{dataset}}/dicomStores/{{name}}' properties: - !ruby/object:Api::Type::String name: pubsubTopic @@ -156,7 +153,6 @@ objects: name: streamConfigs required: false min_version: beta - update_url: '{{dataset}}/dicomStores/{{name}}' description: | To enable streaming to BigQuery, configure the streamConfigs object in your DICOM store. streamConfigs is an array, so you can specify multiple BigQuery destinations. You can stream metadata from a single DICOM store to up to five BigQuery tables in a BigQuery dataset. @@ -243,7 +239,6 @@ objects: identifiers, those IDs will be part of the FHIR resource path recorded in Cloud audit logs and Cloud Pub/Sub notifications. required: false - update_url: '{{dataset}}/fhirStores/{{name}}' - !ruby/object:Api::Type::Boolean name: 'disableReferentialIntegrity' description: | @@ -284,7 +279,6 @@ objects: - !ruby/object:Api::Type::KeyValuePairs name: labels required: false - update_url: '{{dataset}}/fhirStores/{{name}}' description: | User-supplied key-value pairs used to organize FHIR stores. @@ -302,7 +296,6 @@ objects: - !ruby/object:Api::Type::NestedObject name: notificationConfig required: false - update_url: '{{dataset}}/fhirStores/{{name}}' properties: - !ruby/object:Api::Type::String name: pubsubTopic @@ -420,7 +413,6 @@ objects: - !ruby/object:Api::Type::NestedObject name: parserConfig required: false - update_url: '{{dataset}}/hl7V2Stores/{{name}}' properties: - !ruby/object:Api::Type::Boolean name: allowNullHeader @@ -463,7 +455,6 @@ objects: - !ruby/object:Api::Type::KeyValuePairs name: labels required: false - update_url: '{{dataset}}/hl7V2Stores/{{name}}' description: | User-supplied key-value pairs used to organize HL7v2 stores. @@ -516,7 +507,6 @@ objects: removed_message: This field has been replaced by notificationConfigs exact_version: ga required: false - update_url: '{{dataset}}/hl7V2Stores/{{name}}' properties: - !ruby/object:Api::Type::String name: pubsubTopic @@ -534,7 +524,6 @@ objects: deprecation_message: This field has been replaced by notificationConfigs exact_version: beta required: false - update_url: '{{dataset}}/hl7V2Stores/{{name}}' properties: - !ruby/object:Api::Type::String name: pubsubTopic diff --git a/mmv1/templates/terraform/resource.erb b/mmv1/templates/terraform/resource.erb index 075befca63f4..241878528466 100644 --- a/mmv1/templates/terraform/resource.erb +++ b/mmv1/templates/terraform/resource.erb @@ -676,6 +676,7 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ <% end # if !object.input -%> <% if properties_by_custom_update(object.root_properties).length() > 0 -%> d.Partial(true) + <% properties_by_custom_update(object.root_properties) .sort_by {|k, _| k.nil? ? "" : k[:update_id].to_s} .each do |key, props| From 2352dd91300dd59071d0d4b51c810d7fa9d996ab Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 10 Jun 2021 12:19:32 -0700 Subject: [PATCH 5/9] add support for forceNew if version is shrinking --- mmv1/products/redis/terraform.yaml | 2 ++ .../terraform/constants/redis_version.go | 25 +++++++++++++++++++ .../resource_definition/redis_instance.erb | 2 ++ 3 files changed, 29 insertions(+) create mode 100644 mmv1/templates/terraform/constants/redis_version.go create mode 100644 mmv1/templates/terraform/resource_definition/redis_instance.erb diff --git a/mmv1/products/redis/terraform.yaml b/mmv1/products/redis/terraform.yaml index b117bcb54498..93b825f419dd 100644 --- a/mmv1/products/redis/terraform.yaml +++ b/mmv1/products/redis/terraform.yaml @@ -26,6 +26,8 @@ overrides: !ruby/object:Overrides::ResourceOverrides encoder: templates/terraform/encoders/redis_location_id_for_fallback_zone.go.erb decoder: templates/terraform/decoders/redis_instance.go.erb extra_schema_entry: templates/terraform/extra_schema_entry/redis_instance.erb + constants: templates/terraform/constants/redis_version.go + resource_definition: templates/terraform/resource_definition/redis_instance.erb examples: - !ruby/object:Provider::Terraform::Examples name: "redis_instance_basic" diff --git a/mmv1/templates/terraform/constants/redis_version.go b/mmv1/templates/terraform/constants/redis_version.go new file mode 100644 index 000000000000..a17c8e2e058f --- /dev/null +++ b/mmv1/templates/terraform/constants/redis_version.go @@ -0,0 +1,25 @@ +// Is the new redis version less than the old one? +func isRedisVersionDecreasing(_ context.Context, old, new, _ interface{}) bool { + if old == nil || new == nil { + return false + } + re := regexp.MustCompile(`REDIS_(\d+)_(\d+)`) + oldParsed := re.FindSubmatch([]byte(old.(string))) + newParsed := re.FindSubmatch([]byte(new.(string))) + + if oldParsed == nil || newParsed == nil { + // create new if you don't recognize the expression + return true + } + + oldVersion, err := strconv.ParseFloat(fmt.Sprintf("%s.%s", oldParsed[1], oldParsed[2]), 32) + if err != nil { + return false + } + newVersion, err := strconv.ParseFloat(fmt.Sprintf("%s.%s", newParsed[1], newParsed[2]), 32) + if err != nil { + return false + } + + return newVersion < oldVersion +} \ No newline at end of file diff --git a/mmv1/templates/terraform/resource_definition/redis_instance.erb b/mmv1/templates/terraform/resource_definition/redis_instance.erb new file mode 100644 index 000000000000..61ab618a5aa7 --- /dev/null +++ b/mmv1/templates/terraform/resource_definition/redis_instance.erb @@ -0,0 +1,2 @@ +CustomizeDiff: customdiff.All( + customdiff.ForceNewIfChange("redis_version", isRedisVersionDecreasing)), From 8ef10ef4c142a79bfb013fa9bd603b12aaf1c423 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 10 Jun 2021 13:56:44 -0700 Subject: [PATCH 6/9] don't send request if not updating anything.. request fails if update mask is empty.. also add more tests --- mmv1/provider/core.rb | 4 +- .../terraform/constants/redis_version.go | 11 +- mmv1/templates/terraform/resource.erb | 7 + mmv1/templates/terraform/update_mask.erb | 2 +- .../tests/resource_redis_instance_test.go | 144 +++++++++++++++++- 5 files changed, 156 insertions(+), 12 deletions(-) diff --git a/mmv1/provider/core.rb b/mmv1/provider/core.rb index fb38eeec9341..5d7d6a76040e 100644 --- a/mmv1/provider/core.rb +++ b/mmv1/provider/core.rb @@ -328,8 +328,8 @@ def properties_by_custom_update(properties, behavior = :new) # Filter the properties to keep only the ones don't have custom update # method and group them by update url & verb. def properties_without_custom_update(properties) - update_props = properties.reject do |p| - !(p.update_url.nil? || p.update_verb.nil? || p.update_verb == :NOOP) + update_props = properties.select do |p| + p.update_url.nil? || p.update_verb.nil? || p.update_verb == :NOOP end update_props diff --git a/mmv1/templates/terraform/constants/redis_version.go b/mmv1/templates/terraform/constants/redis_version.go index a17c8e2e058f..7411d356e9ff 100644 --- a/mmv1/templates/terraform/constants/redis_version.go +++ b/mmv1/templates/terraform/constants/redis_version.go @@ -1,5 +1,11 @@ + // Is the new redis version less than the old one? func isRedisVersionDecreasing(_ context.Context, old, new, _ interface{}) bool { + return isRedisVersionDecreasingFunc(old, new) +} + +// seperate function for unit testing +func isRedisVersionDecreasingFunc(old, new interface{}) bool { if old == nil || new == nil { return false } @@ -8,8 +14,7 @@ func isRedisVersionDecreasing(_ context.Context, old, new, _ interface{}) bool { newParsed := re.FindSubmatch([]byte(new.(string))) if oldParsed == nil || newParsed == nil { - // create new if you don't recognize the expression - return true + return false } oldVersion, err := strconv.ParseFloat(fmt.Sprintf("%s.%s", oldParsed[1], oldParsed[2]), 32) @@ -22,4 +27,4 @@ func isRedisVersionDecreasing(_ context.Context, old, new, _ interface{}) bool { } return newVersion < oldVersion -} \ No newline at end of file +} diff --git a/mmv1/templates/terraform/resource.erb b/mmv1/templates/terraform/resource.erb index 241878528466..389d463be053 100644 --- a/mmv1/templates/terraform/resource.erb +++ b/mmv1/templates/terraform/resource.erb @@ -645,6 +645,10 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ billingProject = bp } +<% if object.update_mask -%> +// if updateMask is empty we are not updating anything so skip the post +if len(updateMask) > 0 { +<% end -%> res, err := sendRequestWithTimeout(config, "<%= object.update_verb -%>", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutUpdate)<%= object.error_retry_predicates ? ", " + object.error_retry_predicates.join(',') : "" -%>) if err != nil { @@ -673,6 +677,9 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ } <% end -%> <% end -%> +<% if object.update_mask -%> + } +<% end -%> <% end # if !object.input -%> <% if properties_by_custom_update(object.root_properties).length() > 0 -%> d.Partial(true) diff --git a/mmv1/templates/terraform/update_mask.erb b/mmv1/templates/terraform/update_mask.erb index fc5aea548778..5e5359de6f95 100644 --- a/mmv1/templates/terraform/update_mask.erb +++ b/mmv1/templates/terraform/update_mask.erb @@ -20,5 +20,5 @@ if d.HasChange("<%= prop_name %>") { // won't set it url, err = addQueryParams(url, map[string]string{"updateMask": strings.Join(updateMask, ",")}) if err != nil { - return err + return err } diff --git a/mmv1/third_party/terraform/tests/resource_redis_instance_test.go b/mmv1/third_party/terraform/tests/resource_redis_instance_test.go index 09ffa0615da8..225cc4244d2b 100644 --- a/mmv1/third_party/terraform/tests/resource_redis_instance_test.go +++ b/mmv1/third_party/terraform/tests/resource_redis_instance_test.go @@ -18,7 +18,7 @@ func TestAccRedisInstance_update(t *testing.T) { CheckDestroy: testAccCheckRedisInstanceDestroyProducer(t), Steps: []resource.TestStep{ { - Config: testAccRedisInstance_update(name), + Config: testAccRedisInstance_update(name, true), }, { ResourceName: "google_redis_instance.test", @@ -26,13 +26,16 @@ func TestAccRedisInstance_update(t *testing.T) { ImportStateVerify: true, }, { - Config: testAccRedisInstance_update2(name), + Config: testAccRedisInstance_update2(name, true), }, { ResourceName: "google_redis_instance.test", ImportState: true, ImportStateVerify: true, }, + { + Config: testAccRedisInstance_update2(name, false), + }, }, }) } @@ -103,13 +106,100 @@ func TestAccRedisInstance_redisInstanceAuthEnabled(t *testing.T) { }) } -func testAccRedisInstance_update(name string) string { +func TestAccRedisInstance_downgradeRedisVersion(t *testing.T) { + t.Parallel() + + name := fmt.Sprintf("tf-test-%d", randInt(t)) + + vcrTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckRedisInstanceDestroyProducer(t), + Steps: []resource.TestStep{ + { + Config: testAccRedisInstance_redis5(name), + }, + { + ResourceName: "google_redis_instance.test", + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccRedisInstance_redis4(name), + }, + { + ResourceName: "google_redis_instance.test", + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestUnitRedisInstance_redisVersionIsDecreasing(t *testing.T) { + t.Parallel() + type testcase struct { + name string + old interface{} + new interface{} + decreasing bool + } + tcs := []testcase{ + { + name: "stays the same", + old: "REDIS_4_0", + new: "REDIS_4_0", + decreasing: false, + }, + { + name: "increases", + old: "REDIS_4_0", + new: "REDIS_5_0", + decreasing: false, + }, + { + name: "nil vals", + old: nil, + new: "REDIS_4_0", + decreasing: false, + }, + { + name: "corrupted", + old: "REDIS_4_0", + new: "REDIS_banana", + decreasing: false, + }, + { + name: "decreases", + old: "REDIS_6_0", + new: "REDIS_4_0", + decreasing: true, + }, + } + + for _, tc := range tcs { + decreasing := isRedisVersionDecreasingFunc(tc.old, tc.new) + if decreasing != tc.decreasing { + t.Errorf("%s: expected decreasing to be %v, but was %v", tc.name, tc.decreasing, decreasing) + } + } +} + +func testAccRedisInstance_update(name string, preventDestroy bool) string { + lifecycleBlock := "" + if preventDestroy { + lifecycleBlock = ` + lifecycle { + prevent_destroy = true + }` + } return fmt.Sprintf(` resource "google_redis_instance" "test" { name = "%s" display_name = "pre-update" memory_size_gb = 1 region = "us-central1" + %s labels = { my_key = "my_val" @@ -122,15 +212,23 @@ resource "google_redis_instance" "test" { } redis_version = "REDIS_4_0" } -`, name) +`, name, lifecycleBlock) } -func testAccRedisInstance_update2(name string) string { +func testAccRedisInstance_update2(name string, preventDestroy bool) string { + lifecycleBlock := "" + if preventDestroy { + lifecycleBlock = ` + lifecycle { + prevent_destroy = true + }` + } return fmt.Sprintf(` resource "google_redis_instance" "test" { name = "%s" display_name = "post-update" memory_size_gb = 1 + %s labels = { my_key = "my_val" @@ -144,7 +242,7 @@ resource "google_redis_instance" "test" { transit_encryption_mode = "SERVER_AUTHENTICATION" redis_version = "REDIS_5_0" } -`, name) +`, name, lifecycleBlock) } func testAccRedisInstance_regionFromLocation(name, zone string) string { @@ -176,3 +274,37 @@ resource "google_redis_instance" "cache" { } `, context) } + +func testAccRedisInstance_redis5(name string) string { + return fmt.Sprintf(` +resource "google_redis_instance" "test" { + name = "%s" + display_name = "redissss" + memory_size_gb = 1 + region = "us-central1" + + redis_configs = { + maxmemory-policy = "allkeys-lru" + notify-keyspace-events = "KEA" + } + redis_version = "REDIS_5_0" +} +`, name) +} + +func testAccRedisInstance_redis4(name string) string { + return fmt.Sprintf(` +resource "google_redis_instance" "test" { + name = "%s" + display_name = "redissss" + memory_size_gb = 1 + region = "us-central1" + + redis_configs = { + maxmemory-policy = "allkeys-lru" + notify-keyspace-events = "KEA" + } + redis_version = "REDIS_4_0" +} +`, name) +} From 2a2a5ffb07206268d5d9c8021fbc34da77cac0cb Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 11 Jun 2021 14:36:51 -0700 Subject: [PATCH 7/9] fix spelling mistake --- mmv1/provider/core.rb | 6 ++++++ mmv1/templates/terraform/constants/redis_version.go | 2 +- mmv1/templates/terraform/resource.erb | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/mmv1/provider/core.rb b/mmv1/provider/core.rb index 5d7d6a76040e..17803e0dc8a9 100644 --- a/mmv1/provider/core.rb +++ b/mmv1/provider/core.rb @@ -301,6 +301,12 @@ def build_env } end + # used to determine and separate objects that have update methods + # that target individual fields + def has_field_specific_update_methods(properties) + properties_by_custom_update(properties).length() > 0 + end + # Filter the properties to keep only the ones requiring custom update # method and group them by update url & verb. def properties_by_custom_update(properties, behavior = :new) diff --git a/mmv1/templates/terraform/constants/redis_version.go b/mmv1/templates/terraform/constants/redis_version.go index 7411d356e9ff..2231313efe60 100644 --- a/mmv1/templates/terraform/constants/redis_version.go +++ b/mmv1/templates/terraform/constants/redis_version.go @@ -4,7 +4,7 @@ func isRedisVersionDecreasing(_ context.Context, old, new, _ interface{}) bool { return isRedisVersionDecreasingFunc(old, new) } -// seperate function for unit testing +// separate function for unit testing func isRedisVersionDecreasingFunc(old, new interface{}) bool { if old == nil || new == nil { return false diff --git a/mmv1/templates/terraform/resource.erb b/mmv1/templates/terraform/resource.erb index 389d463be053..b934dc03c58b 100644 --- a/mmv1/templates/terraform/resource.erb +++ b/mmv1/templates/terraform/resource.erb @@ -645,7 +645,7 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ billingProject = bp } -<% if object.update_mask -%> +<% if object.update_mask && has_field_specific_update_methods(object.root_properties) -%> // if updateMask is empty we are not updating anything so skip the post if len(updateMask) > 0 { <% end -%> @@ -677,11 +677,11 @@ if len(updateMask) > 0 { } <% end -%> <% end -%> -<% if object.update_mask -%> +<% if object.update_mask && has_field_specific_update_methods(object.root_properties) -%> } <% end -%> <% end # if !object.input -%> -<% if properties_by_custom_update(object.root_properties).length() > 0 -%> +<% if has_field_specific_update_methods(object.root_properties) -%> d.Partial(true) <% properties_by_custom_update(object.root_properties) From 48f2e82e521d8b53005c1b7b6f2446381f944a42 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 11 Jun 2021 15:24:51 -0700 Subject: [PATCH 8/9] fix rake issues --- mmv1/provider/core.rb | 4 ++-- mmv1/templates/terraform/resource.erb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/mmv1/provider/core.rb b/mmv1/provider/core.rb index 17803e0dc8a9..82b2eb12033d 100644 --- a/mmv1/provider/core.rb +++ b/mmv1/provider/core.rb @@ -303,8 +303,8 @@ def build_env # used to determine and separate objects that have update methods # that target individual fields - def has_field_specific_update_methods(properties) - properties_by_custom_update(properties).length() > 0 + def field_specific_update_methods(properties) + properties_by_custom_update(properties).length.positive? end # Filter the properties to keep only the ones requiring custom update diff --git a/mmv1/templates/terraform/resource.erb b/mmv1/templates/terraform/resource.erb index b934dc03c58b..3646081dde6c 100644 --- a/mmv1/templates/terraform/resource.erb +++ b/mmv1/templates/terraform/resource.erb @@ -645,7 +645,7 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ billingProject = bp } -<% if object.update_mask && has_field_specific_update_methods(object.root_properties) -%> +<% if object.update_mask && field_specific_update_methods(object.root_properties) -%> // if updateMask is empty we are not updating anything so skip the post if len(updateMask) > 0 { <% end -%> @@ -677,11 +677,11 @@ if len(updateMask) > 0 { } <% end -%> <% end -%> -<% if object.update_mask && has_field_specific_update_methods(object.root_properties) -%> +<% if object.update_mask && field_specific_update_methods(object.root_properties) -%> } <% end -%> <% end # if !object.input -%> -<% if has_field_specific_update_methods(object.root_properties) -%> +<% if field_specific_update_methods(object.root_properties) -%> d.Partial(true) <% properties_by_custom_update(object.root_properties) From e58cca2d715845cbcafb104ac571cbe642757df8 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Mon, 14 Jun 2021 10:19:29 -0700 Subject: [PATCH 9/9] remove transcription mode... forces new on change --- mmv1/third_party/terraform/tests/resource_redis_instance_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/mmv1/third_party/terraform/tests/resource_redis_instance_test.go b/mmv1/third_party/terraform/tests/resource_redis_instance_test.go index 225cc4244d2b..efa08a45db5a 100644 --- a/mmv1/third_party/terraform/tests/resource_redis_instance_test.go +++ b/mmv1/third_party/terraform/tests/resource_redis_instance_test.go @@ -239,7 +239,6 @@ resource "google_redis_instance" "test" { maxmemory-policy = "noeviction" notify-keyspace-events = "" } - transit_encryption_mode = "SERVER_AUTHENTICATION" redis_version = "REDIS_5_0" } `, name, lifecycleBlock)