Add support for using labels on compute_instance

[selmanj]

Note that the configuration for testAccComputeInstance_update was modified as it was unsetting a value that forced a new instance to be created instead of updated (specifically the metadata_startup_script key).

Part of #17.

[tpoindessous]

Hi

first, I want to thank you for your work. I'm trying to use labels in gcp_compute_snapshot data resource and I will look at your code.

At first sight, it seems that your function func resourceInstanceLabels could be more generic and maybe be useful for other resources.

Is it possible for you, please, to look at this and try to add this function in a more generic file ?

Maybe your other function in _test file could be also used by other people ?

What do you think about this ?

Thank you.

Thomas

[danawillow]

nit: I think it's just as readable to do labels[k] = v.(string)

[selmanj]

whoops, yeah, not sure why there's an extra variable there. Will fix

[danawillow]

Hmm, this checks that a single label exists but doesn't check that there aren't labels we don't expect to be there. Do you think this is sufficient or should it take the key/value map and compare that?

[selmanj]

I could add that but I'm not sure it's something that could happen... Although it might make the test syntax cleaner later down the road.

At some point I think our tests would benefit from building a terraform config up programmatically via a builder or something similar; this would end up making the tests a lot more explicit (currently a lot of information is hidden in the 'config' function). If we had that, then testing the entire map would be a lot easier...

Something like:

	resource.Test(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckComputeInstanceDestroy,
		Steps: []resource.TestStep{
			resource.TestStep{
				Config: NewInstanceBuilder(instanceName).WithLabels(myLabels).Build(),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckComputeInstanceExists(
						"google_compute_instance.foobar", &instance),
					testAccCheckComputeInstanceHasLabels(&instance, myLabels),
				),
			},
		},

[danawillow]

nit: this extra newline got added

[danawillow]

@tpoindessous I mostly agree with you, although I'm not sure where those functions should live. Honestly I think they should be in core somewhere, since unpacking a map[string]interface{} into a map[string]string (or an []interface{} into a []string, which is another one I've seen around a bunch) is something that would be useful across providers.

As for the test one, that particular function itself can't be used by other tests because it's relying specifically on an instance being passed in, but the idea behind it could probably be generalized, sure. I'm not going to block merging on either of these changes since they could pretty easily be done in a follow-up PR by you or @selmanj, but Joe if you want to do them in this one I'm not going to stop you either.

[tpoindessous]

Hi

yes, I agree with you, features first :)

We could change that later, when we will have several usages of Labels.

Thanks for your work, it will be very useful !

[danawillow]

LGTM, thanks!

[ghost]

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!