From 8be5727f3d35229be8ecbd81c025dd5ea8a4ff04 Mon Sep 17 00:00:00 2001 From: Chris Stephens Date: Wed, 22 May 2019 09:55:24 -0700 Subject: [PATCH] Convenience fields of N depth (#1797) Merged PR #1797. --- api/resource.rb | 12 +++ api/type.rb | 12 +++ build/terraform | 2 +- build/terraform-beta | 2 +- build/terraform-mapper | 2 +- overrides/terraform/property_override.rb | 17 +++- products/compute/terraform.yaml | 4 + .../terraform/expand_property_method.erb | 11 ++- .../terraform/flatten_property_method.erb | 15 ++-- templates/terraform/objectlib/base.go.erb | 15 +--- templates/terraform/resource.erb | 83 ++++++------------- 11 files changed, 87 insertions(+), 88 deletions(-) diff --git a/api/resource.rb b/api/resource.rb index 222c79105891..87d0a0baf5bb 100644 --- a/api/resource.rb +++ b/api/resource.rb @@ -219,6 +219,18 @@ def gettable_properties all_user_properties.reject(&:url_param_only) end + # Returns the list of top-level properties once any nested objects with flatten_object + # set to true have been collapsed + def root_properties + properties.flat_map do |p| + if p.flatten_object + p.root_properties + else + p + end + end + end + # Return the product-level async object, or the resource-specific one # if one exists. def async diff --git a/api/type.rb b/api/type.rb index f6259166f432..2ff28d9c98bc 100644 --- a/api/type.rb +++ b/api/type.rb @@ -474,6 +474,18 @@ def nested_properties properties end + # Returns the list of top-level properties once any nested objects with + # flatten_object set to true have been collapsed + def root_properties + properties.flat_map do |p| + if p.flatten_object + p.root_properties + else + p + end + end + end + def exclude_if_not_in_version!(version) super @properties.each { |p| p.exclude_if_not_in_version!(version) } diff --git a/build/terraform b/build/terraform index d6021ef695bc..77de8e286bfc 160000 --- a/build/terraform +++ b/build/terraform @@ -1 +1 @@ -Subproject commit d6021ef695bcc0fb14f36c3712bb664462f0cf5a +Subproject commit 77de8e286bfcf3fbb341061327d3716fb5b8bb01 diff --git a/build/terraform-beta b/build/terraform-beta index 84f7b4812b44..fbeb9de9af5f 160000 --- a/build/terraform-beta +++ b/build/terraform-beta @@ -1 +1 @@ -Subproject commit 84f7b4812b44d453bfbd68eefc56942a8b75cb8e +Subproject commit fbeb9de9af5fda0a65c4697436a3a985f903193f diff --git a/build/terraform-mapper b/build/terraform-mapper index 6b42deda02c7..4a7977e8ac97 160000 --- a/build/terraform-mapper +++ b/build/terraform-mapper @@ -1 +1 @@ -Subproject commit 6b42deda02c70a538a9fb891361a8a1ee02d16df +Subproject commit 4a7977e8ac97e766316da40983dc04494a701088 diff --git a/overrides/terraform/property_override.rb b/overrides/terraform/property_override.rb index cc85b83397fb..ee4f387e3e2f 100644 --- a/overrides/terraform/property_override.rb +++ b/overrides/terraform/property_override.rb @@ -79,13 +79,22 @@ def self.attributes # # With great power comes great responsibility. - # Flatten the child field of a NestedObject into "convenience fields" - # that are addressed as if they were top level fields. + # Flattens a NestedObject by removing that field from the Terraform + # schema but will preserve it in the JSON sent/retreived from the API + # + # EX: a API schema where fields are nested (eg: `one.two.three`) and we + # desire the properties of the deepest nested object (eg: `three`) to + # become top level properties in the Terraform schema. By overidding + # the properties `one` and `one.two` and setting flatten_object then + # all the properties in `three` will be at the root of the TF schema. # # We need this for cases where a field inside a nested object has a # default, if we can't spend a breaking change to fix a misshapen - # field, or if the UX is _much_ better otherwise. Nesting flattened - # NestedObjects is inadvisable. + # field, or if the UX is _much_ better otherwise. + # + # WARN: only fully flattened properties are currently supported. In the + # example above you could not flatten `one.two` without also flattening + # all of it's parents such as `one` :flatten_object, # =========== diff --git a/products/compute/terraform.yaml b/products/compute/terraform.yaml index 6fa40bcb9848..6d5c729cc105 100644 --- a/products/compute/terraform.yaml +++ b/products/compute/terraform.yaml @@ -747,6 +747,10 @@ overrides: !ruby/object:Overrides::ResourceOverrides routingConfig: !ruby/object:Overrides::Terraform::PropertyOverride flatten_object: true routingConfig.routingMode: !ruby/object:Overrides::Terraform::PropertyOverride + # flattened objects don't pass on the attributes of the parent to + # the child so redefining this on the child field is necessary. + update_verb: :PATCH + update_url: projects/{{project}}/global/networks/{{name}} # while required inside routingConfig, routingMode is an optional convenience # field. required: false diff --git a/templates/terraform/expand_property_method.erb b/templates/terraform/expand_property_method.erb index 8aac04fe9906..1e2aff0121ed 100644 --- a/templates/terraform/expand_property_method.erb +++ b/templates/terraform/expand_property_method.erb @@ -43,6 +43,7 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T } return m, nil } +<%# End expanders for Maps %> <%# Generate expanders for KeyValuePairs %> <% elsif property.is_a?(Api::Type::KeyValuePairs) -%> @@ -59,11 +60,11 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T <%# Generate expanders for flattened objects %> <% elsif property.flatten_object -%> -func expand<%= prefix -%><%= titlelize_property(property) -%>(d TerraformResourceData, config *Config) (interface{}, error) { +func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) { transformed := make(map[string]interface{}) <% property.nested_properties.each do |prop| -%> - // Note that nesting flattened objects won't work because we don't handle them properly here. - transformed<%= titlelize_property(prop) -%>, err := expand<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config) + <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%> + transformed<%= titlelize_property(prop) -%>, err := expand<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config) if err != nil { return nil, err <% if prop.send_empty_value -%> @@ -75,8 +76,8 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(d TerraformResourc <% end -%> } - return transformed, nil <% end -%> + return transformed, nil } <%# Generate expanders for everything else %> @@ -158,6 +159,8 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T <% if property.nested_properties? -%> <% property.nested_properties.each do |prop| -%> +<%# Map is a map from {key -> object} in the API, but Terraform can't represent that +so we treat the key as a property of the object in Terraform schema. %> <% next if property.is_a?(Api::Type::Map) && prop.name == property.key_name -%> <%= lines(build_expand_method(prefix + titlelize_property(property), prop), 1) -%> <% end -%> diff --git a/templates/terraform/flatten_property_method.erb b/templates/terraform/flatten_property_method.erb index 8299367311ea..ff15f70dd0e9 100644 --- a/templates/terraform/flatten_property_method.erb +++ b/templates/terraform/flatten_property_method.erb @@ -16,12 +16,6 @@ <%= lines(compile_template(property.custom_flatten, prefix: prefix, property: property)) -%> -<% elsif property.flatten_object -%> -<% if property.nested_properties? -%> - <% property.nested_properties.each do |prop| -%> - <%= lines(build_flatten_method(prefix + titlelize_property(property), prop), 1) -%> - <% end -%> -<% end -%> <% else -%> <% if tf_types.include?(property.class) -%> func flatten<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d *schema.ResourceData) interface{} { @@ -35,8 +29,17 @@ func flatten<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d } transformed := make(map[string]interface{}) <% property.properties.each do |prop| -%> + <% if prop.flatten_object -%> + if <%= prop.api_name -%> := flatten<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(original["<%= prop.api_name -%>"], d); <%= prop.api_name -%> != nil { + obj := <%= prop.api_name -%>.([]interface{})[0] + for k, v := range obj.(map[string]interface{}) { + transformed[k] = v + } + } + <% else -%> transformed["<%= prop.name.underscore -%>"] = flatten<%= prefix -%><%= titlelize_property(property) -%><%= titlelize_property(prop) -%>(original["<%= prop.api_name -%>"], d) + <% end -%> <% end -%> return []interface{}{transformed} <% elsif property.is_a?(Api::Type::Array) && property.item_type.is_a?(Api::Type::NestedObject) -%> diff --git a/templates/terraform/objectlib/base.go.erb b/templates/terraform/objectlib/base.go.erb index e81590d90e81..c2d2843ecf91 100644 --- a/templates/terraform/objectlib/base.go.erb +++ b/templates/terraform/objectlib/base.go.erb @@ -38,18 +38,10 @@ func Get<%= resource_name -%>ApiObject(d TerraformResourceData, config *Config) obj := make(map[string]interface{}) <% object.settable_properties.each do |prop| -%> <% if prop.flatten_object -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config) - if err != nil { - return nil, err -<% unless prop.send_empty_value -%> - } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) { -<% else -%> - } else { -<% end -%> - obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop - } -<% else -%> + <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(nil, d, config) +<% else # if flatten -%> <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config) +<% end -%> if err != nil { return nil, err <% unless prop.send_empty_value -%> @@ -59,7 +51,6 @@ func Get<%= resource_name -%>ApiObject(d TerraformResourceData, config *Config) <% end -%> obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop } -<% end -%> <% end -%> <% if object.custom_code.encoder -%> diff --git a/templates/terraform/resource.erb b/templates/terraform/resource.erb index d64e37ffdcc7..2872cb6b2eb5 100644 --- a/templates/terraform/resource.erb +++ b/templates/terraform/resource.erb @@ -110,29 +110,19 @@ func resource<%= resource_name -%>Create(d *schema.ResourceData, meta interface{ obj := make(map[string]interface{}) <% object.settable_properties.each do |prop| -%> -<% if prop.flatten_object -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config) + <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%> + <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config) if err != nil { return err -<% unless prop.send_empty_value -%> +<% 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 { -<% end -%> - obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop - } <% else -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config) - if err != nil { - return err -<% unless prop.send_empty_value -%> } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) && (ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop)) { -<% else -%> - } else if v, ok := d.GetOkExists("<%= prop.name.underscore -%>"); ok || !reflect.DeepEqual(v, <%= prop.api_name -%>Prop) { -<% end -%> +<% end -%> obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop } -<% end -%> <% end -%> <% if object.custom_code.encoder -%> @@ -262,16 +252,15 @@ func resource<%= resource_name -%>Read(d *schema.ResourceData, meta interface{}) <% object.gettable_properties.reject{|p| p.ignore_read }.each do |prop| -%> <% if prop.flatten_object -%> - if v, ok := res["<%= prop.api_name -%>"].(map[string]interface{}); res["<%= prop.api_name -%>"] != nil && ok { -<% prop.properties.each do |p| -%> - if err := d.Set("<%= p.name.underscore -%>", flatten<%= resource_name -%><%= titlelize_property(prop) -%><%= titlelize_property(p) -%>(v["<%= p.api_name -%>"], d)); err != nil { - return fmt.Errorf("Error reading <%= object.name -%>: %s", err) - } -<% end -%> - } else { -<% prop.properties.each do |p| -%> - d.Set("<%= p.name.underscore -%>", nil) -<% end -%> +// Terraform must set the top level schema field, but since this object contains collapsed properties +// it's difficult to know what the top level should be. Instead we just loop over the map returned from flatten. + if flattenedProp := flatten<%= resource_name -%><%= titlelize_property(prop) -%>(res["<%= prop.api_name -%>"], d); flattenedProp != nil { + casted := flattenedProp.([]interface{})[0] + if casted != nil { + for k, v := range casted.(map[string]interface{}) { + d.Set(k, v) + } + } } <% else -%> if err := d.Set("<%= prop.name.underscore -%>", flatten<%= resource_name -%><%= titlelize_property(prop) -%>(res["<%= prop.api_name -%>"], d)); err != nil { @@ -295,25 +284,13 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ <% if object.input -%> d.Partial(true) -<% properties_by_custom_update(properties).each do |key, props| -%> -<% top_level_props = props.reject(&:flatten_object) + props.select(&:flatten_object).flat_map(&:properties) -%> - if <%= top_level_props.map { |prop| "d.HasChange(\"#{prop.name.underscore}\")" }.join ' || ' -%> { +<% properties_by_custom_update(object.root_properties).each do |key, props| -%> +if <%= props.map { |prop| "d.HasChange(\"#{prop.name.underscore}\")" }.join ' || ' -%> { obj := make(map[string]interface{}) -<% props.each do |prop| -%> -<% if prop.flatten_object -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config) - if err != nil { - return err -<% unless prop.send_empty_value -%> - } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) { -<% else -%> - } else { -<% end -%> - obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop - } -<% else -%> -<% if update_body_properties.include? prop -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config) +<% props.each do |prop| -%> +<% if update_body_properties.include? prop -%> + <% schemaPrefix = prop.flatten_object ? "nil" : "d.Get( \"#{prop.name.underscore}\" )" -%> + <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config) if err != nil { return err <%# There is some nuance in when we choose to send a value to an update function. @@ -344,7 +321,6 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ <%= prop.api_name -%>Prop := d.Get("<%= prop.name.underscore -%>") obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop <% end # if update_body_properties.include? prop -%> -<% end # prop.flatten_object -%> <% end # props.each -%> <% if object.mutex -%> @@ -410,19 +386,9 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ <% else # if object.input -%> obj := make(map[string]interface{}) <% update_body_properties.each do |prop| -%> -<% if prop.flatten_object -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d, config) - if err != nil { - return err -<% unless prop.send_empty_value -%> - } else if !isEmptyValue(reflect.ValueOf(<%= prop.api_name -%>Prop)) { -<% else -%> - } else { -<% end -%> - obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop - } -<% else -%> - <%= prop.api_name -%>Prop, err := expand<%= resource_name -%><%= titlelize_property(prop) -%>(d.Get("<%= prop.name.underscore -%>"), d, config) + <%# 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<%= resource_name -%><%= titlelize_property(prop) -%>(<%= schemaPrefix -%>, d, config) if err != nil { return err <% unless prop.send_empty_value -%> @@ -432,7 +398,6 @@ func resource<%= resource_name -%>Update(d *schema.ResourceData, meta interface{ <% end -%> obj["<%= prop.api_name -%>"] = <%= prop.api_name -%>Prop } -<% end -%> <% end -%> <%# We need to decide what encoder to use here - if there's an update encoder, use that! -%>