New resource: vsphere_tag_category #164

vancluever · 2017-09-19T00:21:19Z

This adds a new resource, vsphere_tag_category, which is the start of
the effort to get tag support into the provider. This allows one to
create and manage tag categories in the provider, which can then be
passed along to tag resources.

As part of this work, other work has been done to prime the provider for
the ability to manage multiple API connections at once, as the provider
now needs to connect to two to do its job. Furthermore, tags are not
supported on all versions of vSphere and not on ESXi, so extra
precautions needed to be undertaken to ensure these cases were properly
blocked.

tombuildsstuff

A few comments but otherwise LGTM

tombuildsstuff · 2017-09-19T08:09:15Z

vendor/vendor.json

@@ -11,8 +17,8 @@
 		{
 			"checksumSHA1": "+2yCNqbcf7VcavAptooQReTGiHY=",
 			"path": "github.com/apparentlymart/go-rundeck-api",


why is Rundeck needed?

govendor add strikes again 😛

I'm going to try this again against my fork using govendor fetch, and see if I get the same deps pulled in.

tombuildsstuff · 2017-09-19T08:43:39Z

vsphere/resource_vsphere_tag_category.go

+			"description": {
+				Type:        schema.TypeString,
+				Description: "The description of the category.",
+				Optional:    true,


should this also be Computed?

Config should be the source of truth, even if there's no description defined, so this should not be computed so that it can be reverted to a blank description if set out-of-band.

tombuildsstuff · 2017-09-19T08:44:40Z

vsphere/resource_vsphere_tag_category.go

+				Elem:        &schema.Schema{Type: schema.TypeString},
+				Required:    true,
+			},
+			"used_by": {


I feel like this is named incorrectly?

Yeah, I almost feel like removing this field altogether. I will do a bit more research on it. API entry where it's referenced.

tombuildsstuff · 2017-09-19T08:46:54Z

vsphere/resource_vsphere_tag_category.go

+	if len(cats) > 1 {
+		// This should not happen but guarding just in case it does.
+		return nil, fmt.Errorf("category name %q returned multiple results, more specific name required", name)
+	}


why not combine these to be:

if len(cats) == 1) { d.SetId(cats[0].ID) return []*schema.ResourceData{d}, nil } return nil, fmt.Errorf("Expected a single category, got '%d'", len(cats))

There's 2 specific cases here - the first one is when an incorrect category is given and the one that will be the one most users will encounter (len(cats) < 1). Technically, it seems impossible for len(cats) > 1 to happen at the moment, as category names need to be unique (adding a category with a different name but different associable types in TF or the UI will result in a name collision error). Yet GetCategoriesByName returns multiple, even though its own logic searches for an exact match. So the second case is just for brevity to match the SDK and I don't expect it to really happen. Regardless, given these two cases are so distinct I'd prefer to have this level of verbosity over a more general len != 1 style error message, which I think is more confusing...

Also as I'm typing this I think the second error is a little misleading too - might end up re-wording this as well, but we still need to distinguish between "not found" (something a user may stumble on if they just mis-typed the name of the category) versus len > 1 (which is more of a change of the expected API and something we will need to fix).

tombuildsstuff · 2017-09-19T08:48:58Z

vsphere/resource_vsphere_tag_category_test.go

+	return func(s *terraform.State) error {
+		_, err := testGetTagCategory(s, "terraform-test-category")
+		if err != nil {
+			if strings.Contains(err.Error(), "Status code: 404") && !expected {


is there no way of getting a concrete object back and checking the status code on that instead?

Nope. The library does not wrap an error type to pull it from (reference).

tombuildsstuff · 2017-09-19T08:53:03Z

website/docs/r/tag_category.html.markdown

+[ext-tags-general]: https://docs.vmware.com/en/VMware-vSphere/6.5/com.vmware.vsphere.vcenterhost.doc/GUID-E8E854DD-AA97-4E0C-8419-CE84F93C4058.html
+[ext-tag-categories]: https://docs.vmware.com/en/VMware-vSphere/6.5/com.vmware.vsphere.vcenterhost.doc/GUID-BA3D1794-28F2-43F3-BCE9-3964CB207FB6.html
+
+~> **NOTE:** Tagging support is unsupported on direct ESXi connections and


this probably also warrants a section on the main vSphere page for "Required vSphere Versions" IMO (although, checking the page perhaps it wants a separate page for that?)

Honestly that index page needs a massive overhaul anyway so I'm not surprised. For now I think I'm fine with it being out of date, but close to v1.0.0 (or during some downtime) I will work on updating it so that not only does it have this information, but the dev info is removed, and other steps are taken to make sure that it focuses on a summary of what the provider does and that's it, saving extra detail for each individual resource.

tombuildsstuff · 2017-09-19T08:54:02Z

website/docs/r/tag_category.html.markdown

+
+* `name` - (String, required) The name of the tag.
+* `description` - (String, optional) A description for the tag.
+* `cardinality` - (String, required, forces new resource) The number of tags


minor in order providers we suffix the description with "Changing this forces a new resource to be created" rather than doing this

Yeah, I'm still saving this work for a documentation overhaul. Keeping consistent with the style that I have on other docs for now, as that will allow me to easier fix this later. I haven't forgotten!

tombuildsstuff · 2017-09-19T08:54:43Z

website/docs/r/tag_category.html.markdown

+  [here](#associable-object-types).
+
+~> **NOTE:** You can add associable types to a category, but you cannot remove
+them. Attempting to do so will result in an error.


sounds like this should be ForceNew?

Adding types is a legitimate operation, removing them is not - so we can't be ForceNew here as that would block the case where someone wants to add a type to a category.

Created a new type, VSphereClient, to hold the multiple client connections we are going to need here now that we are working on using a different SDK for tags.

This adds a new resource, vsphere_tag_category, which is the start of the effort to get tag support into the provider. This allows one to create and manage tag categories in the provider, which can then be passed along to tag resources. As part of this work, other work has been done to prime the provider for the ability to manage multiple API connections at once, as the provider now needs to connect to two to do its job. Furthermore, tags are not supported on all versions of vSphere and not on ESXi, so extra precautions needed to be undertaken to ensure these cases were properly blocked.

Note that this is temporary until vmware/vic#6381 is merged, at which point we will vendor off of the upstream commit.

This reverts commit 5b8a643.

Looks like govendor fetch added this a lot more cleanly than govendor add.

I can't find any sort of useful data on how this is used, the docs don't mention it, the API is murky on its purpose and the UI does not expose it. Adding a tag to an object with the category doesn't add anything to it either. Since we can't figure out what it's used for, I'm just dropping it - if it's missed by any user, we can just add it back later.

For the "multiple category returned" case. By all observations this is a case that can't happen, but VIC's tag package seems to think it can, so we are indicating that the user needs to put in an issue if they indeed somehow managed to add multiple tag categories with the same name.

This reverts commit e036bcb.

Apparently need to add the full package path as the origin :P

vancluever · 2017-09-19T16:12:11Z

Merging now - thanks for the review @tombuildsstuff!

vancluever requested review from tombuildsstuff and mbfrahry September 19, 2017 00:21

vancluever changed the title ~~New resource, vsphere_tag_category~~ New resource": vsphere_tag_category Sep 19, 2017

vancluever changed the title ~~New resource": vsphere_tag_category~~ New resource: vsphere_tag_category Sep 19, 2017

vancluever force-pushed the f-tag-categories branch from ebb2f77 to 5b8a643 Compare September 19, 2017 00:28

tombuildsstuff approved these changes Sep 19, 2017

View reviewed changes

vancluever added 9 commits September 19, 2017 09:01

provider: Move govmomi client to a nested field in client meta

98f987e

Created a new type, VSphereClient, to hold the multiple client connections we are going to need here now that we are working on using a different SDK for tags.

vendor: Add github.com/vmware/vic/pkg/vsphere/tags

3d27574

Note that this is temporary until vmware/vic#6381 is merged, at which point we will vendor off of the upstream commit.

Revert "vendor: Add github.com/vmware/vic/pkg/vsphere/tags"

955c58d

This reverts commit 5b8a643.

vendor: Re-add github.com/vmware/vic/pkg/vsphere/tags

e036bcb

Looks like govendor fetch added this a lot more cleanly than govendor add.

Revert "vendor: Re-add github.com/vmware/vic/pkg/vsphere/tags"

a1fad90

This reverts commit e036bcb.

vendor: Re-add github.com/vmware/vic/pkg/vsphere/tags (again)

0995f64

Apparently need to add the full package path as the origin :P

vancluever force-pushed the f-tag-categories branch from e437e70 to 0995f64 Compare September 19, 2017 16:06

vancluever merged commit 8f28860 into master Sep 19, 2017

vancluever deleted the f-tag-categories branch September 20, 2017 20:03

ghost locked and limited conversation to collaborators Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource: vsphere_tag_category #164

New resource: vsphere_tag_category #164

vancluever commented Sep 19, 2017

tombuildsstuff left a comment

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

tombuildsstuff Sep 19, 2017

vancluever Sep 19, 2017

vancluever commented Sep 19, 2017

New resource: vsphere_tag_category #164

New resource: vsphere_tag_category #164

Conversation

vancluever commented Sep 19, 2017

tombuildsstuff 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

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

vancluever commented Sep 19, 2017