Add basic update for `google_kms_crypto_key` resource #1511

MrSaints · 2018-05-19T00:18:57Z

Prior to this commit, any changes to rotation_period would
force a new resource as no Update was defined for the resource.
This commit introduces a basic Update through calling the
Patch service method. It only modifies the rotation_period,
and next_rotation_time at the moment, but this is reflective
of what is "allowed" on https://console.cloud.google.com/security/kms.

Prior to this commit, any changes to `rotation_period` would force a new resource as no `Update` was defined for the resource. This commit introduces a basic `Update` through calling the `Patch` service method. It only modifies the `rotation_period`, and `next_rotation_time` at the moment, but this is reflective of what is "allowed" on https://console.cloud.google.com/security/kms.

MrSaints · 2018-05-22T11:50:45Z

google/resource_kms_crypto_key.go

+
+	log.Printf("[DEBUG] Updated CryptoKey %s", cryptoKey.Name)
+
+	d.SetId(cryptoKeyId.cryptoKeyId())


I don't think this is entirely necessary (?), but I see this being done across various Terraform providers, so I suppose it can add some guarantee.

Yeah, unless you think it's possible that the Id could change on an update (which in this case, it can't) that doesn't really do anything. I usually don't put it in, but I also don't tell people they have to take it out.

danawillow

Thanks @MrSaints! This looks great- just a few comments/questions.

danawillow · 2018-05-25T00:16:59Z

google/resource_kms_crypto_key.go

+		return err
+	}
+
+	key := cloudkms.CryptoKey{Purpose: "ENCRYPT_DECRYPT"}


Just curious- since it's a PATCH API, do we actually need to include the purpose here since it isn't changing?

Good point - removing.

danawillow · 2018-05-25T00:18:16Z

google/resource_kms_crypto_key.go

+
+	key := cloudkms.CryptoKey{Purpose: "ENCRYPT_DECRYPT"}
+
+	if d.HasChange("rotation_period") && d.Get("rotation_period") != "" {


Hmm, what should the behavior be if someone changes the rotation_period from something to nothing?

In theory, d.Get("rotation_period") will never be an empty string due to validation.
So if we set rotation_period = "", it will fail validation, but that is the master behaviour. If we remove it completely, this block will not execute, but since we included an update mask, it will use the zero value of the struct, so it will end up setting the rotation period to Never. From my understanding, that would've been the case if it was not defined on Create.

Oh, you're totally right- I missed the update mask. Thanks!

danawillow · 2018-05-25T00:20:30Z

google/resource_kms_crypto_key.go

+		cryptoKeyId.cryptoKeyId(),
+		&key,
+	).
+		UpdateMask("rotation_period,next_rotation_time").


nit: We don't have any line-length limitations here, so we tend to go for the "just put it all on one line" style. If that makes you cringe, maybe just put it on two lines or something like that.

danawillow · 2018-05-25T00:21:26Z

google/resource_kms_crypto_key.go

+
+	log.Printf("[DEBUG] Updated CryptoKey %s", cryptoKey.Name)
+
+	d.SetId(cryptoKeyId.cryptoKeyId())


Yeah, unless you think it's possible that the Id could change on an update (which in this case, it can't) that doesn't really do anything. I usually don't put it in, but I also don't tell people they have to take it out.

danawillow · 2018-05-25T00:22:21Z

google/resource_kms_crypto_key.go

@@ -111,6 +112,46 @@ func resourceKmsCryptoKeyCreate(d *schema.ResourceData, meta interface{}) error
 	return resourceKmsCryptoKeyRead(d, meta)
 }

+func resourceKmsCryptoKeyUpdate(d *schema.ResourceData, meta interface{}) error {


Mind adding a test (or adding a step into an existing test) in resource_kms_crypto_key_test.go that does an update?

We are only patching the `rotation_period`, and `next_rotation_time`, so that value will not be affected.

- Test change in rotation period - Test removal of rotation period

MrSaints · 2018-05-25T09:35:21Z

Cheers for the CR @danawillow. I've made the relevant changes.

danawillow · 2018-05-25T15:54:26Z

The test looks like it's failing:

=== RUN   TestAccKmsCryptoKey_rotation
--- FAIL: TestAccKmsCryptoKey_rotation (60.70s)
    testing.go:513: Step 2 error: Check failed: Check 2/2 error: Failed to parse NextRotationTime timestamp: parsing time "" as "2006-01-02T15:04:05.999999999Z07:00": cannot parse "" as "2006"
FAIL

MrSaints · 2018-05-25T16:26:16Z

@danawillow I'll have a look. Strange that Travis CI is passing. Are those tests executed in Travis CI?

danawillow · 2018-05-25T16:27:35Z

No, Travis just does unit tests. Since the acceptance tests create actual cloud resources, we want to be in a bit more control of when they get run.

MrSaints · 2018-05-25T16:55:53Z

Unfortunately, I can't currently run the acceptance tests locally, but I've pushed a quick fix for it. Considering NextRotationTime can be empty, I reckon it is not too ridiculous to shortcircuit early.

danawillow · 2018-05-30T00:17:26Z

Thanks, it's passing now!

I pushed a quick change to your branch to get rid of the ForceNew line altogether (we usually don't bother setting it to false. I figured this was faster than another round of back-and-forth just for the one thing). Merging now, thanks for the contribution!

MrSaints · 2018-05-30T01:09:35Z

Thanks a lot @danawillow! I greatly appreciate it 💯

ghost · 2018-11-18T14:00:09Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

MrSaints commented May 22, 2018

View reviewed changes

danawillow self-requested a review May 25, 2018 00:07

danawillow suggested changes May 25, 2018

View reviewed changes

MrSaints added 3 commits May 25, 2018 10:03

Remove unused Purpose value in CryptoKey

0a82594

We are only patching the `rotation_period`, and `next_rotation_time`, so that value will not be affected.

nit: format Patch operation to be in a single line

096896d

Extend TestAccKmsCryptoKey_rotation test steps

fd00b60

- Test change in rotation period - Test removal of rotation period

Do not parse NextRotationTime if it is not set

d3fd6be

remove ForceNew: false

261d6ee

danawillow approved these changes May 30, 2018

View reviewed changes

danawillow merged commit b4be2fa into hashicorp:master May 30, 2018

MrSaints deleted the resource-kms-crypto-key-add-update branch May 30, 2018 01:09

ghost locked and limited conversation to collaborators Nov 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic update for `google_kms_crypto_key` resource #1511

Add basic update for `google_kms_crypto_key` resource #1511

MrSaints commented May 19, 2018

MrSaints May 22, 2018

danawillow May 25, 2018

danawillow left a comment

danawillow May 25, 2018

MrSaints May 25, 2018

danawillow May 25, 2018

MrSaints May 25, 2018

danawillow May 25, 2018

danawillow May 25, 2018

MrSaints May 25, 2018

danawillow May 25, 2018

danawillow May 25, 2018

MrSaints May 25, 2018

MrSaints commented May 25, 2018

danawillow commented May 25, 2018

MrSaints commented May 25, 2018

danawillow commented May 25, 2018

MrSaints commented May 25, 2018

danawillow commented May 30, 2018

MrSaints commented May 30, 2018

ghost commented Nov 18, 2018


		log.Printf("[DEBUG] Updated CryptoKey %s", cryptoKey.Name)

		d.SetId(cryptoKeyId.cryptoKeyId())


		key := cloudkms.CryptoKey{Purpose: "ENCRYPT_DECRYPT"}

		if d.HasChange("rotation_period") && d.Get("rotation_period") != "" {

Add basic update for google_kms_crypto_key resource #1511

Add basic update for google_kms_crypto_key resource #1511

Conversation

MrSaints commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSaints commented May 25, 2018

danawillow commented May 25, 2018

MrSaints commented May 25, 2018

danawillow commented May 25, 2018

MrSaints commented May 25, 2018

danawillow commented May 30, 2018

MrSaints commented May 30, 2018

ghost commented Nov 18, 2018

Add basic update for `google_kms_crypto_key` resource #1511

Add basic update for `google_kms_crypto_key` resource #1511