Add custom flatten for monitoring channel labels #1251

emilymye · 2019-01-15T20:28:13Z

Also fix some "pertain" license text 😠

Why this is a custom flatten:

DiffSuppressFunc is not called for TypeMap elements in Terraform. We don't want to suppress the entire labels map diff.
CustomizeDiff only allows modification of the actual Diff elements for Computed values, which is not what we want to default to for labels. (Correct me if I am wrong)

Fixes hashicorp/terraform-provider-google#2839

[all]

[terraform]

Add custom flatten for monitoring channel labels

[terraform-beta]

Add custom flatten for monitoring channel labels

[ansible]

[inspec]

rileykarson · 2019-01-15T20:32:41Z

Is this resolving a user's issue? Also -me, @chrisst has the most context here.

emilymye · 2019-01-15T20:38:13Z

Added issue to description

modular-magician · 2019-01-15T20:42:35Z

I am a robot that works on MagicModules PRs!

I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#352
depends: hashicorp/terraform-provider-google#2879

rileykarson · 2019-01-15T20:45:57Z

Thanks!

nat-henderson · 2019-01-15T21:14:25Z

templates/terraform/custom_flatten/monitoring_obfuscated_labels.go.erb

+		}
+
+		replace := true
+		for i := 0; i < len(stateLabel); i++ {


Could this just be a regex match for "serverLabel matches \*+ and len(stateLabel) = len(serverLabel)"?

I suppose so? This is ugly but it matches against the rest of the value that isn't asterisked.

Sorry, what does that mean? Can you post an example of two strings you need to compare?

Am I right in understanding that a len 4 label (for certain labels) submitted to the api will come back obfuscated as a len 4 "****" ?

**CRET vs SECRET (see hashicorp/terraform-provider-google#2839)

It appears that len(secret) <= 4 is left as is. I'm not sure I want to rely on the unobfuscated value always being 4 characters, hence the ugly match, but if you feel like that's an ok thing to rely on, I can change it.

It partially obfuscates them? Goodness. Okay, let me see if I can devise a regex for that ... Whew.

Yeah, my way is doable but worse. Your solution LGTM - please explain in comments, if you don't mind.

I just... wow

chrisst · 2019-01-16T00:44:52Z

I ran into these "sensitive" fields in a different api and never really came up with a perfect solution. My understanding of what you have is "if server sends back stars, overwrite server response with local version". But I think that means you aren't able to tell if the user has actually changed the value of the sensitive label from "foo" to "bar". I ended up with a similar strategy of returning whatever was coming from d.Get instead of the api response for sensitive fields.

Right now the only issue I see is that it's hard to read what is going on. I think a small refactor would clear up what is going on. Maybe something like:

for serverLabel in serverLabels:
if isSensitiveLabel(serverLabel) {
// replace server response
}

it can help reduce the number of break conditions in your loop and you can throw a couple quick tests around the helper method with expected inputs and outputs.

modular-magician · 2019-01-16T17:57:03Z