Enhancement: Added resource for license management

[girishramnani]

Please review the implementation of license resource.

The following capabilities are provided by this resource -

1. Adding license ( with optional labels )
2. Removing license
3. Updating labels attached to a license

Terraform version - v0.9.11
Vsphere version used for acceptance tests - Version 5.5.0 Build 4063694

[vancluever]

Hi @girishramnani! Thank you very much for submitting this PR - it's going to be very useful!

I have been testing this against a standalone ESXi server and there seems to be some issues. I think this stems from the fact that the host API differs somewhat from the vCenter API. There are a couple of formatting issues as well that I hope could be looked at, just to make the tests look a bit better.

I will start a review (expect it in a few hours) and put the relevant information in, but just FYI a good reference to check what needs to happen to support both standalone host and vCenter when working with licenses can be found in the govc license.add call.

If you have any questions or need any help feel free to ask! Looking forward to getting this one merged!

[vancluever]

Hey @girishramnani - just wanted to let you know the review is still pending - sorry for the delay! In the meantime can you check out what I mentioned regarding handling the API that this points against (host vs vCenter) and make the needed changes?

PS: This is the line that needs fixing in Create: https://github.com/terraform-providers/terraform-provider-vsphere/pull/110/files/fb2068625fe0a3c63647156cb53e68e3d28ca7cd#diff-6da7e5115133f8bf52016c3cc74c771cR98

[girishramnani]

Done, so now based on the api type different method to add license will be used.

[vancluever]

Hey @girishramnani! Thanks for the update. I have the rest of the review ready now. Can you look at the points and update/comment accordingly?

I was also wondering if the config generation functions in the tests could be modified as well? I think it might help with readability a lot if the configs were laid out a bit more explicitly and less functionally - maybe something like this:

func testAccVSphereLicenseConfig() {
  return fmt.Sprintf(`
resource "vsphere_license" "foo" {
  license_key = "%s"
}
`, os.Getenv("VSPHERE_LICENSE"))
}

func testAccVSphereLicenseConfig() {
  return fmt.Sprintf(
`resource "vsphere_license" "foo" {
  license_key = "%s"

  label {
    key   = "VpxClientLicenseLabel"
    value = "Hello World"
  }

  label {
    key   = "TestTitle"
    value = "FooBar"
  }
}
`, os.Getenv("VSPHERE_LICENSE"))
}

(Note that the labels will probably change once the label attribute is updated)

Also, I noticed there was a lot of leading spaces in some of the functions in resource_vsphere_license.go, ie: on the first line - example here and here. Maybe it would be a good idea to just go over the code for some possible style artifacts and clean them up?

[vancluever]

Hey @girishramnani, this is where both APIs need to be handled. They may need to be handled in other places as well (depending on how the host API handles labeling). Again, govc's license.add call is a good place to look.

[vancluever]

PS: When updating this you may need to switch the context package you are using to the standard library's version, (ie: import "context" versus import "golang.org/x/net/context") as that is what govmomi uses.

[vancluever]

Just a small nitpick here - you test data (labelStub) contains keys that are not being iterated on in your test - maybe it's possible to trim that and move that fixture to the function too if it's not being consumed by anything else?

[girishramnani]

I think it would be better to add a check for the keys that are not being tested here.

[vancluever]

So you are saying test the entire set? I think that makes sense too - just as long as the fixture as a whole ultimately has use. 🙂

[vancluever]

I'm wondering in what scenario might license_key not be present, especially if it is a Required and ForceNew field? Given these this could probably be omitted, handing this logic off to TF core.

[girishramnani]

Yeah, makes sense. Removing this part.

[vancluever]

Was ForceNew intended here? It doesn't seem that updating a label would require the entire key to be re-added?

[girishramnani]

Wouldn't ForceNew here mean that if keyof this label changes then that should create a new label instead of updating it?

[vancluever]

I notice that labels are not updated here - would it be a good idea to do so so that any drift caught on the labels can be reconciled?

[girishramnani]

If i update the labels here, then the state provided by terraform files and terraform state stored will drift apart, for example if server has labels x:y and local state shows it as x:yy then the server state has to be changed not the other way around. That's why only computed properties are set as they are the one which the user is not responsible for.

[vancluever]

This is actually the intended behaviour of Terraform - it should detect this kind of drift (ie: if a user manually adds a label) and bring it back to the correctly configured state. It encourages management to be done out of configuration, treating it as the source of truth.

When someone wants to ignore drift, the correct thing to do in a Terraform configuration is to add ignore_changes for the attributes they want to ignore:

resource "vsphere_license" "license" {
  ...

  lifecycle {
    ignore_changes = ["label"]
  }
}

[girishramnani]

But then in the next apply terraform will detect a difference in the local
state and the state specified by the terraform files which will trigger an
update. Due to this labels added by the user manually would be removed. Am
I correct?

[vancluever]

Hey @girishramnani, that is correct, but as mentioned above, this is intended Terrafrom behaviour - resources should always be set up so that they reconcile state with config, even if resources were intentionally modified by a user outside of Terraform. ignore_changes is explicitly used to disregard such changes when that's intentional.

[vancluever]

I think this could probably do with more detail. Something like:

• label - (Optional) A set of labels to apply to the license key. May be specified multiple times to supply multiple label keys.

The values for label are:

• key - (Required) The key for the label.
• values - (Required) A list of string values to assign to the label key.

The latter note on values also takes into consideration my notes on the labels attribute in the schema itself - so check there for more details.

[girishramnani]

values is not a list of string, value is a string. Does the api support multiple values for a single key?

[vancluever]

I think my confusion was in this part:

One key can have multiple labels

I did double check as well, and a label can't have multiple values - sorry about that! So I think what you are saying here is that a in particular license can have multiple labels, right? I think in that case, it might be a good idea to just drop this sentence altogether, considering the double meaning of key (license key and label key) may cause confusion.

[girishramnani]

instead of key, I can write license_key can have multiple labels

[vancluever]

Can this section be cleaned up a bit too? Something like:

• edition_key - The product edition of the license key.
• total - Total number of units (example: vCPUs) contained in the license.
• used - The number of units (example: vCPUs) assigned to this license.
• name - The display name for the license.

[vancluever]

Can this be changed to the standard library context? So just context, versus golang.org/x/net/context.

[vancluever]

Noticed that you mention that labels can be have multiple values - can this be switched to a TypeSet so that all values can be specified in a single resource?

[girishramnani]

I don't think one label can have multiple values. Where have I mentioned that?

[vancluever]

I was confused about this - sorry again! I put some notes in the documentation comment relevant to this.

[vancluever]

This should be a TypeSet so that conflicting entries can't be added.

Also, you will need a hashing function as well when converting it to a set - the stock HashResource function should help too.

[vancluever]

PS: In light of the misunderstanding over label data - this could possibly be moved to a TypeMap to make things easier as well, seeing as no complex resource stuff needs to happen on this being that it's just a set of key/value pairs.

[girishramnani]

Could you please clarify which approach is going to be used here, TypeMap or TypeSet

[girishramnani]

@vancluever thank you for such a quick and thorough review. I have done the changes according to the comments could you please review them.

[vancluever]

Looking good here! Thank you for the fix!

Just a couple of remaining nitpicks on the labels line:

• Can we indicate that it's a map now?
• Can we say key/value instead of key value?
• It says has to be attached, but it's optional.

Something like:

labels - (Optional) A map of key/value pairs to be attached as labels (tags) to the license key.

[vancluever]

We are getting close here - remember though that we don't necessarily need to render the labels here dynamically - it's better to just use a static string, and roll the function into a much more simpler form:

func testAccVSphereLicenseWithLabelConfig() string {
  return fmt.Sprintf(`
resource "vsphere_license" "foo" {
  license_key = "%s"

  labels {
    VpxClientLicenseLabel = "Hello World"
    TestTitle = "FooBar"
  }  
}
`, os.Getenv("VSPHERE_LICENSE"))
}

[vancluever]

As per the label generator here, remember we should style this function the same way:

func testAccVSphereLicenseWithLabelConfig() string {
  return fmt.Sprintf(`
resource "vsphere_license" "foo" {
  license_key = "%s"
}
`, os.Getenv("VSPHERE_LICENSE"))
}

[vancluever]

Hey @girishramnani thanks for the updates! Added some comments inline for the docs and tests stuff that still needs to be adjusted.

The following items are still outstanding:

• Ensure labels are being saved to state on Read calls
• Fix spurious whitespace at the start of functions - there are still several places that have them - it would be useful to go over the whole set of changes and address this. One was also added in 8f6e127. I think maybe there's a misunderstanding that these lines should be removed, not added.
• Just had a couple of nitpicks about language on the labels attribute and then that's good to go.
• We are making progress on the tests but I still would like to see the config generation functions cleaned up a bit.

After this work, this also needs to be fully tested against an ESXi host. From what I read in the LicenseManager object, tags are ignored by AddLicense on ESXi, which might be causing the errors I was seeing in these tests on ESXi (the test returned tag not found after a successful call to add the license).

Thank you for your patience and continued work on this!

[girishramnani]

@vancluever thanks once again for the review, I have changed the code according to the comments, could you please review it.

[vancluever]

Hey @girishramnani - making more progress!

Here are the current test results:

vCenter

=== RUN   TestAccVSphereLicenseBasic
--- PASS: TestAccVSphereLicenseBasic (3.32s)
=== RUN   TestAccVSphereLicenseInvalid
--- PASS: TestAccVSphereLicenseInvalid (0.99s)
=== RUN   TestAccVSphereLicenseWithLabels
--- FAIL: TestAccVSphereLicenseWithLabels (0.00s)
	testing.go:428: Step 0 error: Error loading configuration: Error parsing /tmp/tf-test756435681/main.tf: At 6:21: Unknown token: 6:21 IDENT FooBar

Standalone ESXi

This first round of tests was with a license that I don't think is valid for ESXi at all (vSAN). I think better error trapping needs to happen to ensure that invalid license types for a node (not just invalid keys) are caught.

=== RUN   TestAccVSphereLicenseBasic
--- FAIL: TestAccVSphereLicenseBasic (1.35s)
	testing.go:428: Step 0 error: Error applying: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: The key was not found
	testing.go:492: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error refreshing: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: vsphere_license.foo: The key was not found
		
		State: <nil>
=== RUN   TestAccVSphereLicenseInvalid
--- FAIL: TestAccVSphereLicenseInvalid (1.31s)
	testing.go:422: Step 0, expected error:
		
		Error applying: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: The key was not found
		
		To match:
		
		License file not found
		
		
	testing.go:492: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error refreshing: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: vsphere_license.foo: The key was not found
		
		State: <nil>
=== RUN   TestAccVSphereLicenseWithLabels
--- FAIL: TestAccVSphereLicenseWithLabels (0.00s)
	testing.go:428: Step 0 error: Error loading configuration: Error parsing /tmp/tf-test872458016/main.tf: At 6:21: Unknown token: 6:21 IDENT FooBar

This second test was with an Enterprise Plus key. This one succeeds minus the issues with labels and the broken config that is causing issues with all of the tests (see below).

=== RUN   TestAccVSphereLicenseBasic
--- PASS: TestAccVSphereLicenseBasic (3.28s)
=== RUN   TestAccVSphereLicenseInvalid
--- FAIL: TestAccVSphereLicenseInvalid (1.32s)
	testing.go:422: Step 0, expected error:
		
		Error applying: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: The key was not found
		
		To match:
		
		License file not found
		
		
	testing.go:492: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.
		
		Error: Error refreshing: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: vsphere_license.foo: The key was not found
		
		State: <nil>
=== RUN   TestAccVSphereLicenseWithLabels
--- FAIL: TestAccVSphereLicenseWithLabels (0.00s)
	testing.go:428: Step 0 error: Error loading configuration: Error parsing /tmp/tf-test762915504/main.tf: At 6:21: Unknown token: 6:21 IDENT FooBar

So looks like the config needs to be fixed on the label test, and also the outstanding label tests need to be corrected against the ESXi hosts (might be an idea to kick back an error if a user is trying to define labels on an ESXi server versus vCenter, to prevent confusion).

Also, the formatting needs to be changed on your config generation functions still (again, change them to a simple form where the raw string is not indented and you are using os.Getenv directly in the fmt.Sprintf call).

Thank you for the continued attention on this one!

[vancluever]

Hey @girishramnani, while not necessarily a panic that affects users, this panics when there is an error setting up the client (ie: if one forgets to skip SSL verification). Can you add something to trap nil in this situation and return an error?

[girishramnani]

@vancluever So should I change it in all the places? Like other tests and methods.

[girishramnani]

Hello @vancluever, I have solved the invalid config. A small doubt, could you please help me understand why TestAccVSphereLicenseInvalid test is failing in cases of VSAN key and enterprise key? This test doesn't even depend on the license key.

Also could you please provide me the govc response debug xml files when you used the VSAN key so that I can add that in the error check (i.e. DecodeError method).

[vancluever]

Hey @girishramnani, I think the issues with both the invalid license tests and the issues with adding the VSAN key might be due to the differences between the vCenter and host APIs. I tried a completely invalid key and got the same kind of error. Just using govc license.add -dump ABCDE-11111-11111-11111-11111:

vCenter

The error that the code currently looks for is displayed here correctly.

(license.licenseOutput) (len=1 cap=1) {
    (types.LicenseManagerLicenseInfo) {
        DynamicData: (types.DynamicData) {
        },
        LicenseKey: (string) "",
        EditionKey: (string) "",
        Name: (string) "",
        Total: (int32) 0,
        Used: (int32) 0,
        CostUnit: (string) "",
        Properties: ([]types.KeyAnyValue) (len=4 cap=4) {
            (types.KeyAnyValue) {
                DynamicData: (types.DynamicData) {
                },
                Key: (string) (len=8) "lc_error",
                Value: (int32) 5
            },
            (types.KeyAnyValue) {
                DynamicData: (types.DynamicData) {
                },
                Key: (string) (len=19) "localizedDiagnostic",
                Value: (types.LocalizableMessage) {
                    DynamicData: (types.DynamicData) {
                    },
                    Key: (string) (len=38) "com.vmware.vim.vc.license.error.decode",
                    Arg: ([]types.KeyAnyValue) (len=1 cap=4) {
                        (types.KeyAnyValue) {
                            DynamicData: (types.DynamicData) {
                            },
                            Key: (string) (len=10) "diagnostic",
                            Value: (string) (len=19) "licenseFileNotFound"
                        }
                    },
                    Message: (string) (len=22) "License file not found"
                }
            },
            (types.KeyAnyValue) {
                DynamicData: (types.DynamicData) {
                },
                Key: (string) (len=10) "diagnostic",
                Value: (string) (len=37) "License is not valid for this product"
            },
            (types.KeyAnyValue) {
                DynamicData: (types.DynamicData) {
                },
                Key: (string) (len=9) "Localized",
                Value: (types.ArrayOfKeyAnyValue) {
                    KeyAnyValue: ([]types.KeyAnyValue) <nil>
                }
            }
        },
        Labels: ([]types.KeyValue) <nil>
    }
}

ESXi

Here, the error isn't, so the code is assuming that the add is successful. This is the same dump profile I get when trying to add the VSAN key.

(license.licenseOutput) (len=1 cap=1) {
    (types.LicenseManagerLicenseInfo) {
        DynamicData: (types.DynamicData) {
        },
        LicenseKey: (string) (len=29) "ABCDE-11111-11111-11111-11111",
        EditionKey: (string) "",
        Name: (string) "",
        Total: (int32) 0,
        Used: (int32) 0,
        CostUnit: (string) "",
        Properties: ([]types.KeyAnyValue) (len=46 cap=64) {
            (types.KeyAnyValue) {
                DynamicData: (types.DynamicData) {
                },
                Key: (string) (len=10) "diagnostic",
                Value: (string) (len=37) "License is not valid for this product"
            },
            (types.KeyAnyValue) {
                DynamicData: (types.DynamicData) {
                },
                Key: (string) (len=11) "ProductName",
                Value: (string) (len=17) "VMware ESX Server"
            },
        ...
    }
}

It looks like both errors have the common "License is not valid for this product" un-localized diagnostic KeyAnyValue - maybe we should filter off of that instead? If we do that, I think there's a good chance all of these tests will pass.

PS: Looks like the raw strings for the config generation functions still need to be fixed. Remember that it needs to be un-indented. Example: this config should be generated more like:

func testAccVSphereLicenseWithLabelConfig() string {
  return fmt.Sprintf(`
resource "vsphere_license" "foo" {
  license_key = "%s"
  labels {
    VpxClientLicenseLabel = "Hello World"
    TestTitle = "FooBar"
  }		 	
}
`, os.Getenv("VSPHERE_LICENSE"))
}

PPS: Also, can you rebase this PR off of master and re-vendor the license packages of gvmomi off of v0.15.0? Both of these things have changed since this PR was submitted.

[girishramnani]

Hello @vancluever , I have added error and Error checks for ESXi server, removed indentation and checked for invalid license instead of decode.error. Could you please review it.

[vancluever]

Sorry I missed this before @girishramnani but this needs to be changed to CPUs as well - I don't think I've seen a license have vCPUs at all.

[girishramnani]

Sure, changing.

[girishramnani]

Hello @vancluever , changed the docs are per the request.

[vancluever]

Hey @girishramnani, getting close now. Tests all pass on ESXi, but now on vCenter we get:

=== RUN   TestAccVSphereLicenseBasic
--- PASS: TestAccVSphereLicenseBasic (2.10s)
=== RUN   TestAccVSphereLicenseInvalid
--- PASS: TestAccVSphereLicenseInvalid (0.54s)
=== RUN   TestAccVSphereLicenseWithLabels
--- FAIL: TestAccVSphereLicenseWithLabels (0.61s)
	testing.go:422: Step 0, expected error:

		Error applying: 1 error(s) occurred:

		* vsphere_license.foo: 1 error(s) occurred:

		* vsphere_license.foo: No description for lc_error code

		To match:

		Labels are not allowed in ESXi


FAIL
exit status 1

I think this is the source of the issue - can we split the tests up into vSphere and ESXi-specific tests? Maybe let's add the VSPHERE_TEST_ESXI environment variable to indicate if we are testing against ESXi or not and skip a tests based on the results.

The last item would be the test code - the raw strings are still indented - sorry if this is not 100% clear but I need it laid out in a fashion similar to this. If there's maybe something keeping you from doing this (ie: IDE issues), we can just merge this and make the change after the fact. Let me know!

[girishramnani]

Hello @vancluever , Sorry about the space issue. I have split the test in VCenter and ESXi specific part and also removed the extra space. Thank you for your patience on this PR.

[vancluever]

Hey @girishramnani - getting closer!

Test results:

vCenter

=== RUN   TestAccVSphereLicenseBasic
--- PASS: TestAccVSphereLicenseBasic (3.37s)
=== RUN   TestAccVSphereLicenseInvalid
--- PASS: TestAccVSphereLicenseInvalid (0.89s)
=== RUN   TestAccVSphereLicenseWithLabelsOnVCenter
--- FAIL: TestAccVSphereLicenseWithLabelsOnVCenter (0.91s)
	testing.go:428: Step 0 error: Error applying: 1 error(s) occurred:
		
		* vsphere_license.foo: 1 error(s) occurred:
		
		* vsphere_license.foo: No description for lc_error code
=== RUN   TestAccVSphereLicenseWithLabelsOnESXiServer
--- SKIP: TestAccVSphereLicenseWithLabelsOnESXiServer (0.00s)
	resource_vsphere_license_test.go:109: VSPHERE_TEST_ESXI must be set to true for this acceptance test

ESXi

=== RUN   TestAccVSphereLicenseBasic
--- PASS: TestAccVSphereLicenseBasic (3.31s)
=== RUN   TestAccVSphereLicenseInvalid
--- PASS: TestAccVSphereLicenseInvalid (0.79s)
=== RUN   TestAccVSphereLicenseWithLabelsOnVCenter
--- SKIP: TestAccVSphereLicenseWithLabelsOnVCenter (0.00s)
	resource_vsphere_license_test.go:103: VSPHERE_TEST_ESXI must not be set for this acceptance test
=== RUN   TestAccVSphereLicenseWithLabelsOnESXiServer
--- PASS: TestAccVSphereLicenseWithLabelsOnESXiServer (0.71s)

Can you review what might be going on with TestAccVSphereLicenseWithLabelsOnVCenter and fix accordingly?

Also - still need to fix one config generator (it could actually be moved to a constant). I will comment it.

Thanks!

[vancluever]

Hey @girishramnani, looks like this one is still indented - might be worthwhile to just move this to a constant too as nothing in it is being generated.

[girishramnani]

Hello @vancluever, I cannot reproduce the lc_error on VCenter could you please share the verbose logs.

[girishramnani]

Hello @vancluever, this is the output of my tests

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run License -timeout 120m
?       bitbucket.org/girishramnani/terraform_vsphere    [no test files]
=== RUN   TestAccVSphereLicenseBasic
--- PASS: TestAccVSphereLicenseBasic (1.47s)
=== RUN   TestAccVSphereLicenseInvalid
--- PASS: TestAccVSphereLicenseInvalid (0.16s)
=== RUN   TestAccVSphereLicenseWithLabelsOnVCenter
--- PASS: TestAccVSphereLicenseWithLabelsOnVCenter (0.78s)
=== RUN   TestAccVSphereLicenseWithLabelsOnESXiServer
--- SKIP: TestAccVSphereLicenseWithLabelsOnESXiServer (0.00s)
        resource_vsphere_license_test.go:115: VSPHERE_TEST_ESXI must be set to true for this acceptance test
PASS
ok      bitbucket.org/girishramnani/terraform_vsphere/vsphere    2.455s

Vsphere version used for acceptance tests - Version 5.5.0 Build 4063694. I am not able to reproduce the error, could you please provide me the govc debug logs.

[vancluever]

Hey @girishramnani - interesting! I am testing against vSphere 6.5 U1, so there definitely is a version discrepancy there that we may need to investigate. I will make a note to get the info you need tomorrow morning!

[vancluever]

Hey @girishramnani, here's an XML of one the request's output:

<?xml version="1.0" encoding="UTF-8"?>
<soapenv:Envelope xmlns:soapenc="http://schemas.xmlsoap.org/soap/encoding/" xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <soapenv:Body>
    <AddLicenseResponse xmlns="urn:vim25">
      <returnval>
        <licenseKey/>
        <editionKey/>
        <name/>
        <total>0</total>
        <costUnit/>
        <properties>
          <key>lc_error</key>
          <value xsi:type="xsd:int">-1</value>
        </properties>
        <properties>
          <key>localizedDiagnostic</key>
          <value xsi:type="LocalizableMessage">
            <key>com.vmware.vim.vc.license.error.decode</key>
            <arg>
              <key>diagnostic</key>
              <value xsi:type="xsd:string">unknownError</value>
            </arg>
            <message>Unknown license decode error</message>
          </value>
        </properties>
        <properties>
          <key>diagnostic</key>
          <value xsi:type="xsd:string">No description for lc_error code</value>
        </properties>
        <properties>
          <key>Localized</key>
          <value xsi:type="ArrayOfKeyAnyValue"/>
        </properties>
      </returnval>
    </AddLicenseResponse>
  </soapenv:Body>
</soapenv:Envelope>

This is happening on creation. I kind of wonder if we need to work around this by doing an initial create without the labels and then update it afterwards. Source. Will check and then update the issue.

[vancluever]

Hey @girishramnani, looks like this was indeed the issue.

Here is a very non-optimal sketch of my updated workflow:

		info, err = manager.Add(context.TODO(), key, nil)
		if err != nil {
			return err
		}
		if err = DecodeError(info); err != nil {
			return err
		}
		d.SetId(info.LicenseKey)
		return resourceVSphereLicenseUpdate(d, meta)

After this, the test passes:

=== RUN   TestAccVSphereLicenseWithLabelsOnVCenter
--- PASS: TestAccVSphereLicenseWithLabelsOnVCenter (4.32s)

I think there is some re-factoring that could be done too. I will add annotations.

[vancluever]

Since labels can't be used on ESXi period, it might be good to just remove finalLabels here and just use nil.

[vancluever]

As mentioned in the PR main comments, this needs to be re-factored into a two-step process - one where the license is added, and another where the labels are added afterwards. I think what you could probably do is break off part of the logic in Update into its own function, and then call out to it here. There is possibly some other re-factoring that could be done here too, considering the possible divergence of workflows.

[vancluever]

Again, this is the logic that should probably be spun off into its own function so it can be used in Create as well to add the labels as a separate step after the initial addition of the license is performed.

[girishramnani]

Done

[girishramnani]

Hello @vancluever, I have updated the code according to your comments could you please review it.

[girishramnani]

Hello @vancluever, the above commit a small fix which adds an error check statement for add license.

[girishramnani]

Rebasing

[girishramnani]

Hello @vancluever, thank you for merging master. Not rebasing.

[vancluever]

@girishramnani - of course - it's important to us that work gets preserved! I needed to fix the conflicts on the website doc index and wanted to make sure I didn't have to force push.

This now LGTM! I am merging this shortly - but thank you very much for your patience on this one and your response to my feedback! I will be releasing a new version of the provider next week which will have your work in it 👍

Thanks again!