Org VDC import, data source and read fix #324

vbauzys · 2019-09-06T13:20:51Z

Continuing changed for supporting data sources and imports.

Org VDC import added
Data source for Org VDC
Fixed read issue with complex types

Signed-off-by: Vaidotas Bauzys <[email protected]>

…vider-vcd

…roviders/terraform-provider-vcd

…vider-vcd

Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd

…roviders/terraform-provider-vcd

…vider-vcd

…roviders/terraform-provider-vcd

Signed-off-by: Vaidotas Bauzys <[email protected]>

…roviders/terraform-provider-vcd

Signed-off-by: Vaidotas Bauzys <[email protected]> # Conflicts: # CHANGELOG.md

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder

Second pass

dataclouder · 2019-09-12T12:21:18Z

vcd/resource_vcd_org_vdc.go

+// capacityResource return schema for capacity
+func capacitySchema() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeSet,


Given that the set has only one element, why don't we use the simpler TypeList instead?
That would save us the aggravation of detecting hash values while testing, and the decrease in test readability.

TypeSet handles naturally that only one type of resource is configured

+1 for this. We would just need to check if we are not breaking compatibility with previous version. At the moment this is forcing every user using this data source to do ${tolist(tolist(data.vcd_org_vdc.existingVdc.compute_capacity)[0].cpu)[0].allocated} to pull out some the value.

If everyone agrees please create new Issue and new PR

dataclouder · 2019-09-12T12:27:18Z

vcd/resource_vcd_org_vdc_test.go

@@ -24,6 +24,7 @@ func TestAccVcdOrgVdcReservationPool(t *testing.T) {
 		"AllocationModel":           "ReservationPool",
 		"ProviderVdc":               testConfig.VCD.ProviderVdc.Name,
 		"NetworkPool":               testConfig.VCD.ProviderVdc.NetworkPool,
+		"Allocated":                 "2048",


We are adding an allocated value to compute_capacity.0.cpu.0.allocated but we don't check the value in the tests. Same for the deep values of memory.

Added checks for all test.

dataclouder · 2019-09-12T12:47:37Z

vcd/datasource_vcd_org_vdc_test.go

+			{"memory", memoryHashInternalValue, "reserved"},
+			{"memory", memoryHashInternalValue, "used"},
+		}
+		for _, td := range testData {


Why one value is accessed with zero indexes, while the other needs the calculated hashes?
(I know that I suggested the code simplification, but the original went in the same way)

if attr["compute_capacity.0.memory.0.reserved"] != vdcResource.Primary.Attributes[fmt.Sprintf("compute_capacity.%s.memory.%s.reserved", mainHashValue, memoryHashInternalValue)] { return fmt.Errorf("compute_capacity.0.memory.0.reserved is %#v; want %#v", attr["compute_capacity.0.memory.0.reserved"], vdcResource.Primary.Attributes[fmt.Sprintf("compute_capacity.%s.memory.%s.reserved", mainHashValue, memoryHashInternalValue)]) }

This question goes to Hashicorp. Seems that datasource for some reason didn't use hashes

Signed-off-by: Vaidotas Bauzys <[email protected]>

vbauzys · 2019-09-17T13:21:12Z

Added convertion changes from typeSet to typeList

Didainius

The list thing looks much cleaner! Thanks for the effort

vcd/resource_vcd_org_vdc.go

Didainius · 2019-09-17T14:07:10Z

CHANGELOG.md

@@ -22,6 +23,8 @@ IMPROVEMENTS:
 * `resource/vcd_vapp_vm` allows to force guest customization [#310]
 * `resource/vcd_vapp` supports guest properties [#319]
 * `resource/vcd_vapp_vm` supports guest properties [#319]
+* Upgrade Terraform SDK dependency to 0.12.6 [#302]
+* `vcd_org_vdc` Add import capability - [#324]


Worth noting it gets the "read" capability as someone may be surprised getting changes after regular terraform apply

website/docs/d/org_vdc.html.markdown

vcd/resource_vcd_org_vdc.go

Didainius · 2019-09-18T04:25:41Z

vcd/api_test.go

+// Given "parent.1234.child.5678.something", it will return "1234" and "5678"
+// for Example: compute_capacity.315866465.memory.508945747.limit
+//              (returns 315866465 and 508945747)
+func getHashValuesFromKey(stateFileMap map[string]string, parentKey, childKey string) (string, string, error) {


I guess now this gets to a question if this is used at all. Although it may come in handy in some other place in future.

Didainius · 2019-09-18T04:28:20Z

vcd/datasource_vcd_org_vdc_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccVcdVdcDatasource(t *testing.T) {


I get the impression, that this test does not test the data source and its fields, but vice versa. That is - it uses a data source to generate some vdc config and checks that the resource is provisioned as specified from the data source. Wondering if this can mask some issues with data source itself (for example if it does not retrieve some fields correctly but resource provisioning still works)

This test according Giuseppes recommendations, his done previously his investigation. It's best approach though it has own issues.

One way to improve on this approach is to use output constructs in the test, and compare them with known items in the existing setup.
For example, we can use output to show the provider VDC, and compare that one with the name from our testing configuration.

output "provider_vdc" { value = data.vcd_org_vdc.existingVdc.provider_vdc_name }

in the test, then use

resource.TestCheckOutput("provider_vdc", testConfig.VCD.ProviderVdc.Name),

See some examples in PR #333

Sorry don't get idea - what is difference if I would check : resource.TestCheckResourceAttr("vcd_org_vdc."+vdcName, "provider_vdc_name", testConfig.VCD.ProviderVdc.Name)
Why we would need output?

Your approach is equivalent.
Using output shows explicitly that we are accessing the data source in the HCL script.

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder

Some provisional comments. Running tests now

vcd/datasource_vcd_org_vdc.go

dataclouder · 2019-09-18T08:01:30Z

vcd/datasource_vcd_org_vdc_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccVcdVdcDatasource(t *testing.T) {


One way to improve on this approach is to use output constructs in the test, and compare them with known items in the existing setup.
For example, we can use output to show the provider VDC, and compare that one with the name from our testing configuration.

output "provider_vdc" { value = data.vcd_org_vdc.existingVdc.provider_vdc_name }

in the test, then use

resource.TestCheckOutput("provider_vdc", testConfig.VCD.ProviderVdc.Name),

See some examples in PR #333

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder · 2019-09-18T12:37:25Z

vcd/resource_vcd_org_vdc.go

+func resourceVcdOrgVdcImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
+	resourceURI := strings.Split(d.Id(), ".")
+	if len(resourceURI) != 2 {
+		return nil, fmt.Errorf("resource name must be specified as org.vdc.my_existing_vdc")


There is an extra element in the example path:
"resource name must be specified as org.vdc.my_existing_vdc" should be
"resource name must be specified as org.my_existing_vdc"

good catch, fixed

website/docs/d/org_vdc.html.markdown

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder

LGTM
Good job! This was a tough one!

lvirbalas

Beautiful @vbauzysvmware and finally a solid LGTM!

vbauzys added 12 commits July 15, 2019 15:39

Add missing file

6585559

Signed-off-by: Vaidotas Bauzys <[email protected]>

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

e247d25

…vider-vcd

git push origin masterMerge branch 'master' of github.com:terraform-p…

cf1e7c2

…roviders/terraform-provider-vcd

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

be6790f

…vider-vcd

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

6f4c5cb

…vider-vcd

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

c73d402

…vider-vcd

git push origin master

261d142

Merge branch 'master' of github.com:terraform-providers/terraform-provider-vcd

git push origin masterMerge branch 'master' of github.com:terraform-p…

c431d43

…roviders/terraform-provider-vcd

git push origin masterMerge branch 'master' of github.com:terraform-p…

414b3bf

…roviders/terraform-provider-vcd

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

8b03ab8

…vider-vcd

git push origin masterMerge branch 'master' of github.com:terraform-p…

36c5487

…roviders/terraform-provider-vcd

First implementation - import, data source, read fix

db397a2

Signed-off-by: Vaidotas Bauzys <[email protected]>

ghost added size/XL documentation labels Sep 6, 2019

vbauzys mentioned this pull request Sep 9, 2019

Add function GetStorageProfileByHref and fix bug in type vmware/go-vcloud-director#240

Merged

vbauzys added 5 commits September 9, 2019 11:53

Moved function to govcd

9180dc2

Signed-off-by: Vaidotas Bauzys <[email protected]>

git push origin masterMerge branch 'master' of github.com:terraform-p…

db01eec

…roviders/terraform-provider-vcd

Merge branch 'master' into import_vdc

638d286

Signed-off-by: Vaidotas Bauzys <[email protected]> # Conflicts: # CHANGELOG.md

Fix according latest govcd

4db6be4

Signed-off-by: Vaidotas Bauzys <[email protected]>

Change to use new StringMap introduced

558c726

Signed-off-by: Vaidotas Bauzys <[email protected]>

vbauzys requested review from Didainius, dataclouder and lvirbalas and removed request for Didainius, dataclouder and lvirbalas September 9, 2019 09:36

vbauzys self-assigned this Sep 9, 2019

vbauzys requested a review from lvirbalas September 9, 2019 09:36

vbauzys added 2 commits September 12, 2019 15:19

Add comment

9f643aa

Signed-off-by: Vaidotas Bauzys <[email protected]>

Add comment

4d67b97

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder suggested changes Sep 12, 2019

View reviewed changes

dataclouder reviewed Sep 12, 2019

View reviewed changes

vbauzys added 3 commits September 12, 2019 16:02

Improved test

71e54a1

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improve comment

af69dbf

Signed-off-by: Vaidotas Bauzys <[email protected]>

Convert from typeSet to typeList

1de8c02

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius requested changes Sep 18, 2019

View reviewed changes

vbauzys added 2 commits September 18, 2019 07:40

Improvements

e0dad07

Signed-off-by: Vaidotas Bauzys <[email protected]>

Removed not used anymore

7a6bd51

Signed-off-by: Vaidotas Bauzys <[email protected]>

ghost added size/XL and removed size/XXL labels Sep 18, 2019

Improve tests

847e4d8

Signed-off-by: Vaidotas Bauzys <[email protected]>

ghost added size/XXL and removed size/XL labels Sep 18, 2019

Improve tests

c46bb79

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder suggested changes Sep 18, 2019

View reviewed changes

Improve tests

d2468fb

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder reviewed Sep 18, 2019

View reviewed changes

website/docs/d/org_vdc.html.markdown Show resolved Hide resolved

vbauzys added 4 commits September 18, 2019 16:03

Add import documentation

72adf38

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fixes

7b87d2b

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added output

83a9b0f

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added supported version

f6bfd64

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder approved these changes Sep 18, 2019

View reviewed changes

Didainius approved these changes Sep 18, 2019

View reviewed changes

lvirbalas approved these changes Sep 18, 2019

View reviewed changes

vbauzys merged commit 1ed718d into vmware:master Sep 18, 2019

dataclouder mentioned this pull request Sep 27, 2019

Add data source support #218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Org VDC import, data source and read fix #324

Org VDC import, data source and read fix #324

vbauzys commented Sep 6, 2019 •

edited

Loading

dataclouder left a comment

dataclouder Sep 12, 2019

vbauzys Sep 12, 2019

Didainius Sep 12, 2019

vbauzys Sep 12, 2019

dataclouder Sep 12, 2019

vbauzys Sep 12, 2019

dataclouder Sep 12, 2019

vbauzys Sep 12, 2019

vbauzys commented Sep 17, 2019

Didainius left a comment

Didainius Sep 17, 2019

vbauzys Sep 18, 2019

Didainius Sep 18, 2019

Didainius Sep 18, 2019

vbauzys Sep 18, 2019

dataclouder Sep 18, 2019 •

edited

Loading

vbauzys Sep 18, 2019

dataclouder Sep 18, 2019

dataclouder left a comment

dataclouder Sep 18, 2019 •

edited

Loading

dataclouder Sep 18, 2019

vbauzys Sep 18, 2019

dataclouder left a comment

lvirbalas left a comment

Org VDC import, data source and read fix #324

Org VDC import, data source and read fix #324

Conversation

vbauzys commented Sep 6, 2019 • edited Loading

dataclouder 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

vbauzys commented Sep 17, 2019

Didainius 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

dataclouder Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

dataclouder Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

vbauzys commented Sep 6, 2019 •

edited

Loading

dataclouder Sep 18, 2019 •

edited

Loading

dataclouder Sep 18, 2019 •

edited

Loading