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

Data Catalog tag template #3555

Conversation

danawillow
Copy link
Contributor

Part of hashicorp/terraform-provider-google#5892

Release Note Template for Downstream PRs (will be copied)

`data_catalog_tag_template`

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 6 files changed, 1059 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 1059 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 200 insertions(+))
TF OiCS: Diff ( 4 files changed, 141 insertions(+))

@danawillow danawillow requested a review from megan07 May 21, 2020 22:20
@danawillow
Copy link
Contributor Author

Open questions:

  • Right now I've set force=true for delete all the time, but should it be its own field so people set it explicitly?
  • TF doesn't have support for string -> object maps, which is why fields are implemented as a set of objects (with a "key" field of field_id). Will the tag resources need to reference anything in here? If so we may need to think about how they would do that, since elements of sets aren't easily referenceable

Copy link
Contributor

@megan07 megan07 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@megan07
Copy link
Contributor

megan07 commented May 26, 2020

Open questions:

  • Right now I've set force=true for delete all the time, but should it be its own field so people set it explicitly?
  • TF doesn't have support for string -> object maps, which is why fields are implemented as a set of objects (with a "key" field of field_id). Will the tag resources need to reference anything in here? If so we may need to think about how they would do that, since elements of sets aren't easily referenceable

Sorry, I missed this.

  • From my understanding, force=true was currently the only option. Would adding it as a field in the future be considered a breaking change, as long as we default to true? If it'd be considered breaking, then maybe we should add it now and document it in the same was as the API has it documented
  • I've got the tags working with this set up so far, so I think we're good - if you see anything while reviewing the Tag resource, we might have to reconsider, but I think it will be fine :)

@danawillow
Copy link
Contributor Author

force=true is the only option that lets it get deleted successfully; otherwise it errors. It's basically the inverse of a deletion_protection field (since it defaults to false). I'm fairly inclined to just add it now (maybe call it force_delete?) after all the bigtable hullabaloo.

@megan07
Copy link
Contributor

megan07 commented May 26, 2020

Sure! That sounds good to me!

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 6 files changed, 1073 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 1073 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 200 insertions(+))
TF OiCS: Diff ( 4 files changed, 143 insertions(+))

@@ -84,6 +84,11 @@ overrides: !ruby/object:Overrides::ResourceOverrides
primary_resource_id: "basic_tag_template"
vars:
tag_template_id: "my_template"
force_delete: "false"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is for the docs, correct? do we want to put false in there yet before it's supported by the API? I don't have a strong opinion either way, just wanted to check thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, same rationale here as the bigtable one where I prefer to recommend the safer option for users and make them explicitly have to switch over to the one that lets them delete the resource when they decide to do so.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 5 files changed, 946 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 946 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 200 insertions(+))
TF OiCS: Diff ( 4 files changed, 143 insertions(+))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants