Add Custom Attribute resource and data source #229

rmbrad · 2017-10-31T22:26:59Z

This allows management of Custom Attributes and allows their values to be specified on VMs satisfying #120. Specifically it adds a vsphere_custom_attribute data source for using predefined attributes, a vsphere_custom_attribute resource for managing new custom attributes using Terraform, and a custom_attributes argument on VM resources for specifying custom attribute values.

Still need to add documentation and tests, which I can work on if you're interested in merging.

vancluever · 2017-10-31T22:45:20Z

Wow @rmbrad, this would be definitely something that I think we would be interested in - it would be a great compliment to the current tagging support! 👍

I will hold off on my review until you have time to finish the tests and the docs. 🙂

And thank you!

rmbrad · 2017-11-08T23:58:56Z

@vancluever Finally got around to finishing off the documentation and tests. Let me know if there's anything I need to update after you get a chance to review.

vancluever · 2017-11-18T00:36:12Z

Hey @rmbrad - just had a minute to go over the docs now. I noticed that the only resource to have custom attribute added to it so far has been virtual machines. Can you work to add the support to other resources?

It might help to spin off the common functionality like we do with tags - see tags_helper.go.

Once #244 is in we will need to re-visit adding this to the new VM resource, so this will probably go in sometime after that gets merged.

Thanks!

rmbrad · 2017-11-20T17:58:05Z

Hi @vancluever. No problem, I'll start adding support for other resources this week.

vancluever · 2017-12-05T23:04:12Z

@rmbrad I just did a quick once-over on your progress and I just wanted to say it's looking awesome! 👍

Looks like you are making some pushes still so I'll wait till it's all settled before I start the review - let me know when you are ready!

vancluever · 2017-12-05T23:07:04Z

PS: One thing I did want to say/ask - have you tested this at all on ESXi? I have to double check, but if custom attributes are not supported on ESXi (as in the same fashion as tags), we need to make sure we skip this functionality and prohibit its use when connected to ESXi directly. There is a small helper in internal/helper/viapi that can be used to detect non-vCenter connections and send a generic error message.

Looking forward to getting this in!

rmbrad · 2017-12-07T17:41:55Z

@vancluever Thanks! I just need to add support for the datastore resources to achieve parity with the tag support. I've been running all tests against vCenter and not ESXi directly, but will be setting up a new ESXi instance for testing the VMFS datastore support; so that will allow me to verify whether ESXi supports custom attributes or not.

I'm hoping to have everything ready for review by early next week, at this point it's only the testing that takes time as all logic is now in the helper.

rmbrad · 2017-12-12T17:37:06Z

Hey @vancluever - I believe this PR should now be ready for review. I've added support for custom attributes to all resources that supported tagging. Custom attributes are unsupported on direct ESXi connections, so I added the necessary checks similar to what is done for tagging support.

Let me know if there's anything else needed.

vancluever

Hey @rmbrad - this looks really great - well done!

I have some questions and change requests inline before I go ahead and merge and smoke test it with some real-world configs.

One thing outside of this that I just want to double check - have you tested removing all tags from a resource, after they have been added? This was a pitfall we missed on tags I want to make sure we don't necessarily miss again.

I will make a note to review this again in the next couple of days (including actually testing it with real config).

Thanks for your patience with timing on this as well!

vancluever · 2017-12-15T18:20:29Z

vsphere/internal/helper/customattribute/custom_attributes_helper.go

+	"github.com/vmware/govmomi/vim25/types"
+)
+
+// vSphereCustomAttributeKey is the string that should always be used as the


Looks like this needs to be changed to match the name of the identifier:

// ConfigKey is the string...

vancluever · 2017-12-15T18:21:12Z

vsphere/internal/helper/customattribute/custom_attributes_helper.go

+// resources.
+const ConfigKey = "custom_attributes"
+
+// customAttributesSchema returns the schema for custom attribute configuration


Same here

// ConfigSchema returns the schema...

vancluever · 2017-12-15T18:31:34Z

vsphere/resource_vsphere_datacenter_test.go

+
+resource "vsphere_datacenter" "testDC" {
+  name = "testDC"
+  custom_attributes = "${map(vsphere_custom_attribute.terraform-test-attribute.id, "value")}"


I notice the pattern of using the map interpolation function is present in tests quite a bit. I'm not too sure if this works, but have you just tried:

resource "vsphere_datacenter" "testDC" { name = "testDC" custom_attributes = { "${vsphere_custom_attribute.terraform-test-attribute.id}" = "value" } }

I would prefer that if it works - leaving more complicated scenarios using interpolation to any individual practitioner's use case 🙂

Yeah, use of the map function bothers me as well; but unfortunately interpolation doesn't work for map keys in resources as mentioned here hashicorp/terraform#14516 (comment) 😞.

One work around I often use is to place the definition of the custom attribute map in a locals block, where map keys are interpolated (keys will also be interpolated when passing to a module) as follows:

locals { dc_attrs = { "${vsphere_custom_attribute.terraform-test-attribute.id}" = "value" } } resource "vsphere_datacenter" "testDC" { name = "testDC" custom_attributes = ${local.dc_attrs} }

If you would prefer I can update the tests to work that way.

I think this looks a lot cleaner 🙂 and we also use the map assignment pattern in other places like you type there (not necessarily in the vSphere provider, but elsewhere in TF-land).

vancluever · 2017-12-15T18:36:21Z

vsphere/resource_vsphere_distributed_port_group_test.go

@@ -171,6 +171,53 @@ func TestAccResourceVSphereDistributedPortGroup(t *testing.T) {
 				},
 			},
 		},
+		{
+			"single_attribute",


I think we need to be more descriptive in the sub-tests here - tag is descriptive enough, but attribute has meaning in TF itself - ie: a resource attribute. Maybe something like "single custom attribute", etc.

vancluever · 2017-12-15T18:49:26Z

website/docs/r/datacenter.html.markdown

@@ -46,6 +46,12 @@ The following arguments are supported:
 ~> **NOTE:** Tagging support is unsupported on direct ESXi connections and
 requires vCenter 6.0 or higher.

+* `custom_attributes` - (Optional) Map of custom attribute ids to value 


Can we have a link to directions on how to apply custom attributes similar to what we have for tags here?

vancluever · 2017-12-15T18:51:27Z

website/docs/d/custom_attribute.html.markdown

+
+## Argument Reference
+
+* `name` - (String, required) The name of the custom attribute.


We recently standardized on a new naming convention for attributes that I want to make sure is followed - the VM resoruce has a good layout of the format. An example is below:

some_attr - (Required or Optional) Does some thing. Forces a new resource if changed (if this actually is a ForceNew) argument.

rmbrad · 2017-12-15T23:57:39Z

Hi @vancluever, I've rebased the PR on master and believe I have addressed all your comments, with exception of the use of the map function I commented on above. I also added a test to the vsphere_folder resource that removes all custom attribute values. Let me know if you find any other issues. Thanks!

Just a couple of fixes to the example, formatting fixes and a shuffle of where the links are in the sidebar to keep the alphabetical arrangement.

vancluever · 2017-12-18T23:26:15Z

@rmbrad I've done my final review and just make a couple of documentation corrections, but it largely looked good!

I had a second thought about the tests regarding keeping the map interpolation function as I actually think that looks better after all - but I noticed you changed the tests already. That's fine - we can change it on a refactor if it rubs anyone the wrong way later on. I want to keep the convention of using map for examples and stuff though, and it looks like you've kept that, so we are good.

Anyway, thank you very much for this - this is a great PR and we appreciate the contribution! Well done!

rmbrad force-pushed the f-custom-attrs branch 3 times, most recently from 390846d to d77778b Compare November 8, 2017 23:53

vancluever added enhancement Type: Enhancement new-data-source Feature: New Data Source new-resource Feature: New Resource labels Nov 18, 2017

rmbrad force-pushed the f-custom-attrs branch 7 times, most recently from 6d37a4c to a1a1be9 Compare December 5, 2017 22:35

rmbrad force-pushed the f-custom-attrs branch 2 times, most recently from adebca9 to e8dc3f0 Compare December 12, 2017 17:31

vancluever self-assigned this Dec 15, 2017

vancluever suggested changes Dec 15, 2017

View reviewed changes

Add custom attribute data source and resource

1d50693

rmbrad force-pushed the f-custom-attrs branch from e8dc3f0 to 6ee86c1 Compare December 15, 2017 23:39

rmbrad added 2 commits December 18, 2017 11:53

Add custom attribute support to folder resource

92b288f

Add custom attribute support to datacenter resource

a3469d8

rmbrad added 5 commits December 18, 2017 12:30

Add custom attribute support to VM resource

c968353

Add custom attribute support to virtual switch resources

aabc2a9

Add custom attribute support to port group resources

e911d85

Add custom attribute support to VMFS Datastore resource

3e2ba87

Add custom attribute support to NAS datastore resource

9116fb9

rmbrad force-pushed the f-custom-attrs branch from 6ee86c1 to 9116fb9 Compare December 18, 2017 18:02

docs: Small corrections to custom attrs docs

54905f4

Just a couple of fixes to the example, formatting fixes and a shuffle of where the links are in the sidebar to keep the alphabetical arrangement.

vancluever approved these changes Dec 18, 2017

View reviewed changes

vancluever merged commit 92eb982 into hashicorp:master Dec 18, 2017

vancluever mentioned this pull request Mar 5, 2018

Feature Request: Manage Custom Attributes for VMS (annotations) #120

Closed

ghost locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Custom Attribute resource and data source #229

Add Custom Attribute resource and data source #229

rmbrad commented Oct 31, 2017

vancluever commented Oct 31, 2017 •

edited

Loading

rmbrad commented Nov 8, 2017

vancluever commented Nov 18, 2017

rmbrad commented Nov 20, 2017

vancluever commented Dec 5, 2017

vancluever commented Dec 5, 2017

rmbrad commented Dec 7, 2017

rmbrad commented Dec 12, 2017

vancluever left a comment

vancluever Dec 15, 2017

vancluever Dec 15, 2017

vancluever Dec 15, 2017

rmbrad Dec 15, 2017

vancluever Dec 16, 2017

vancluever Dec 15, 2017

vancluever Dec 15, 2017

vancluever Dec 15, 2017

rmbrad commented Dec 15, 2017 •

edited

Loading

vancluever commented Dec 18, 2017


		## Argument Reference

		* `name` - (String, required) The name of the custom attribute.

Add Custom Attribute resource and data source #229

Add Custom Attribute resource and data source #229

Conversation

rmbrad commented Oct 31, 2017

vancluever commented Oct 31, 2017 • edited Loading

rmbrad commented Nov 8, 2017

vancluever commented Nov 18, 2017

rmbrad commented Nov 20, 2017

vancluever commented Dec 5, 2017

vancluever commented Dec 5, 2017

rmbrad commented Dec 7, 2017

rmbrad commented Dec 12, 2017

vancluever 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

rmbrad commented Dec 15, 2017 • edited Loading

vancluever commented Dec 18, 2017

vancluever commented Oct 31, 2017 •

edited

Loading

rmbrad commented Dec 15, 2017 •

edited

Loading