add data catalog tag

[megan07]

Addresses hashicorp/terraform-provider-google#5892

It's very possible this doesn't require all the custom code and that I'm just missing some settings in api.yaml/terraform.yaml

Release Note Template for Downstream PRs (will be copied)

`data_catalog_tag`

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 1627 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 1627 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 216 insertions(+))
TF OiCS: Diff ( 12 files changed, 565 insertions(+))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 1630 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 7 files changed, 1630 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 220 insertions(+))
TF OiCS: Diff ( 12 files changed, 565 insertions(+))

[danawillow]

What happens without the expander? Does it just get the name wrong?

[megan07]

yea, i thought this was strange too. i thought maybe the issue was that i'm renaming the nested value to enum_value, but looking closer I wonder if it's because it's a set? and calling d.Get("enum_value") should be expanded to d.Get("fields.xxxx.enum_value")?
The request sends, when really displayName has a value

		"pii_type": {
			"enum_value": {
				"displayName": null
			}
		}

And appears to be in issue where v is the correct value that we want, but the autogenerated code calls d.Get("enum_value") which in turn is nil

	transformed := make(map[string]interface{})
	transformedEnumValue, err := expandNestedDataCatalogTagFieldsEnumValueEnumValue(d.Get("enum_value"), d, config)
	if err != nil {
		return nil, err
	} else if val := reflect.ValueOf(transformedEnumValue); val.IsValid() && !isEmptyValue(val) {
		transformed["display_name"] = transformedEnumValue
	}

	return transformed, nil

I tried hard-coding it and it seemed to work, so i think this might be necessary. Unless there's a way to do it without a custom_expand?

[danawillow]

ah yeah I see where the code is doing that but I don't have an immediate solution. I'm fine with the custom expand as long as it's well-commented why it's needed.

[danawillow]

I'm not able to figure out from this why we have to read the template to see which attribute to populate rather than just doing it based off of which one was set by the user. Since our expanders check for emptiness, there shouldn't be anything in the object to delete.

[megan07]

This is the request being sent if I don't have this encoder in here:

		"pii_type": {
			"bool_value": false,
			"display_name": "",
			"double_value": 0,
			"enum_value": {
				"display_name": "EMAIL"
			},
			"order": 0,
			"string_value": "",
			"timestamp_value": ""
		},

I'm maybe missing something somewhere else in the code where this would be circumvented, but in this case we only want enum_value.display_name populated, we don't want to send the other values. They seem to be added in expandNestedDataCatalogTagFields where it's not really checking if it's nil and not adding it, it's still adding it to the map, despite it being nil.

[danawillow]

https://github.com/GoogleCloudPlatform/magic-modules/blob/master/templates/terraform/expand_property_method.erb#L76-L82 I think if that logic is duplicated up into the "generate expanders for maps" section, that would do the trick

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 1689 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 9 files changed, 1689 insertions(+), 24 deletions(-))
TF Conversion: Diff ( 1 file changed, 220 insertions(+))
TF OiCS: Diff ( 12 files changed, 571 insertions(+))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 1713 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 9 files changed, 1713 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 2 files changed, 230 insertions(+))
TF OiCS: Diff ( 12 files changed, 571 insertions(+))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 1668 insertions(+), 17 deletions(-))
Terraform Beta: Diff ( 12 files changed, 1668 insertions(+), 17 deletions(-))
TF Conversion: Diff ( 6 files changed, 213 insertions(+), 14 deletions(-))
TF OiCS: Diff ( 12 files changed, 571 insertions(+))

[danawillow]

LGTM though I'm curious about the custom flattener.

Also since this affects a bunch of resources due to the expander change, can you keep an eye on those tests?

[danawillow]

Can you add a comment on what this is doing that the generated flattener doesn't?

[megan07]

@danawillow - thanks for mentioning this, i looked further into it and although the documentation has the fields snake case (which i originally wrote them as), i tried them with camel case and it worked, so i switched them there. that's originally what the custom flattener was for. i repurposed it now specifically for enumValue because without the custom flattener it returns an array of interfaces rather than the string it's expecting. hope that makes sense!

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 1695 insertions(+), 17 deletions(-))
Terraform Beta: Diff ( 12 files changed, 1695 insertions(+), 17 deletions(-))
TF Conversion: Diff ( 6 files changed, 213 insertions(+), 14 deletions(-))
TF OiCS: Diff ( 12 files changed, 571 insertions(+))

[modular-magician]

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=118748"