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

fixed error when a boolean field set to false #4004

Merged
merged 1 commit into from
Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions products/datacatalog/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ objects:
Holds the value for a tag field with string type.
- !ruby/object:Api::Type::Boolean
name: boolValue
send_empty_value: true
description: |
Holds the value for a tag field with boolean type.
- !ruby/object:Api::Type::String
Expand Down
16 changes: 15 additions & 1 deletion products/datacatalog/terraform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,19 @@ overrides: !ruby/object:Overrides::ResourceOverrides
force_delete: "true"
oics_vars_overrides:
force_delete: "true"
- !ruby/object:Provider::Terraform::Examples
name: "data_catalog_entry_tag_false"
primary_resource_id: "basic_tag"
vars:
entry_group_id: "my_entry_group"
entry_id: "my_entry"
tag_template_id: "my_template"
force_delete: "false"
test_vars_overrides:
force_delete: "true"
oics_vars_overrides:
force_delete: "true"
skip_docs: true # omitting doc as it is almost identical to the case of data_catalog_entry_tag_basic
properties:
# Changing the name here so when mm generates methods like `flattenDataCatalogTagTemplateDisplayName`
# this doesn't conflict with tag template's display name methods
Expand All @@ -173,9 +186,10 @@ overrides: !ruby/object:Overrides::ResourceOverrides
required: false
custom_code: !ruby/object:Provider::Terraform::CustomCode
custom_import: templates/terraform/custom_import/data_catalog_tag.go.erb
encoder: templates/terraform/encoders/data_catalog_tag.go.erb
# This is for copying files over
files: !ruby/object:Provider::Config::Files
# These files have templating (ERB) code that will be run.
# This is usually to add licensing info, autogeneration notices, etc.
compile:
<%= lines(indent(compile('provider/terraform/product~compile.yaml'), 4)) -%>
<%= lines(indent(compile('provider/terraform/product~compile.yaml'), 4)) -%>
17 changes: 17 additions & 0 deletions templates/terraform/encoders/data_catalog_tag.go.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
if obj["fields"] != nil {
// isEmptyValue() does not work for a boolean as it shows
// false when it is 'empty'. Filter boolValue here based on
// the rule api does not take more than 1 'value'
fields := obj["fields"].(map[string]interface{})
for _, elements := range fields {
values := elements.(map[string]interface{})
if len(values) > 1 {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to remove user-set boolValues, and even ones that are set to True? Even though we are enacting the API's rules, we should either validate during plan or let the user's bad configuration return an error, not change their values underneath the hood.

I think this encoder solves the problem of sending too many boolValue's, but checking if the user has set the field using d.GetOkExists() would do better here to make sure we are carrying out the user's intent. We can check if the value is set, and if not, delete it. How does that sound?

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 see.

Copy link
Member

Choose a reason for hiding this comment

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

Actually backtracking on this: we can't use d.GetOkExists() because fields is a set and cannot be indexed. Don't really know any other way of tell the difference between user set/unset. Do you @ndmckinley ?

We stick with this original approach and only remove if the bool_value is false? Seems slightly better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing sticks out to me. Maybe we should pause this for a day or two and think on it, let it percolate, bring it up in the next Monday meeting if we haven't thought of anything clever by then. What do you think?

Copy link
Contributor Author

@edwardmedia edwardmedia Sep 23, 2020

Choose a reason for hiding this comment

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

good to me

Copy link
Member

Choose a reason for hiding this comment

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

Does the API only except 1 value for fields ? If so we can use One-of validation to make sure the user isn't setting multiple values. Then we are free to delete the extra boolean values in this encoder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the API only take 1 value for each fields. But there is no way to distinguish between empty and false for bool, along with other value types.

for val := range values {
if val == "boolValue" {
delete(values, "boolValue")
}
}
}
}
}
return obj, nil
64 changes: 64 additions & 0 deletions templates/terraform/examples/data_catalog_entry_tag_false.tf.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
resource "google_data_catalog_entry" "entry" {
entry_group = google_data_catalog_entry_group.entry_group.id
entry_id = "<%= ctx[:vars]['entry_id'] %>"

user_specified_type = "my_custom_type"
user_specified_system = "SomethingExternal"
}

resource "google_data_catalog_entry_group" "entry_group" {
entry_group_id = "<%= ctx[:vars]['entry_group_id'] %>"
}

resource "google_data_catalog_tag_template" "tag_template" {
tag_template_id = "<%= ctx[:vars]['tag_template_id'] %>"
region = "us-central1"
display_name = "Demo Tag Template"

fields {
field_id = "source"
display_name = "test boolean value"
type {
primitive_type = "BOOL"
}
is_required = true
}

fields {
field_id = "num_rows"
display_name = "Number of rows in the data asset"
type {
primitive_type = "DOUBLE"
}
}

fields {
field_id = "pii_type"
display_name = "PII type"
type {
enum_type {
allowed_values {
display_name = "EMAIL"
}
allowed_values {
display_name = "SOCIAL SECURITY NUMBER"
}
allowed_values {
display_name = "NONE"
}
}
}
}

force_delete = "<%= ctx[:vars]['force_delete'] %>"
}

resource "google_data_catalog_tag" "<%= ctx[:primary_resource_id] %>" {
parent = google_data_catalog_entry.entry.id
template = google_data_catalog_tag_template.tag_template.id

fields {
field_name = "source"
bool_value = false
}
}