VDC supports Flex #285

vbauzys · 2020-01-22T14:30:02Z

Ref:https://github.com/terraform-providers/terraform-provider-vcd/issues/423, https://github.com/terraform-providers/terraform-provider-vcd/pull/443
Enhances existing implementation to allow create VDC allocation model Flex:

refactored and introduced structure to work with different versions of API using same public function.
added Flex and new fields
added unit tests

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

Didainius

Had a look over. There is still a conflict for merge and some inline comments.

govcd/adminorg.go

govcd/adminvdc.go

govcd/api_vcd_versions.go

govcd/adminvdc_test.go

Didainius · 2020-01-30T05:54:25Z

CHANGELOG.md

@@ -4,6 +4,8 @@
 * Added methods `VM.AddInternalDisk`, `VM.GetInternalDiskById`, `VM.DeleteInternalDisk`, `VM.UpdateInternalDisks` and `VM.UpdateInternalDisksAsync`
 * Change `int` and `bool` fields from types.VAppTemplateLeaseSettings and VAppLeaseSettings into pointers
 * Added method `catalog.GetVappTemplateByHref`, and expose methods `vdc.GetEdgeGatewayByHref` and `vdc.GetEdgeGatewayRecordsType`
+* Added methods `adminOrg.CreateOrgVdc`, `adminOrg.CreateOrgVdcAsync` and improved existing to support Flex VDC model


I would say we need a sentence that there is a dynamic method and version change invocation behind the scenes based on vCD version

govcd/adminvdc.go

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

Signed-off-by: Vaidotas Bauzys <[email protected]> # Conflicts: # govcd/org_test.go

vbauzys · 2020-01-30T06:28:36Z

Had a look over. There is still a conflict for merge and some inline comments.

Conflicts solved

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

lvirbalas

First comments.

govcd/adminvdc.go

govcd/api_vcd_versions.go

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

dataclouder

Some small changes required

dataclouder · 2020-02-05T09:00:14Z

govcd/adminvdc.go

 	if err != nil {
 		return AdminVdc{}, err
 	}

-	err = task.WaitTaskCompletion()
+	realFunction, ok := vdcProducerByVersion["vdc"+apiVersionToVcdVersion[apiVersion]]


Rather than realFunction, a better name here would be producer

dataclouder · 2020-02-05T09:00:54Z

govcd/adminvdc.go

+	if err != nil {
+		return Task{}, err
+	}
+	realFunction, ok := vdcProducerByVersion["vdc"+apiVersionToVcdVersion[apiVersion]]


Rather than realFunction, a better name here would be producer

dataclouder · 2020-02-05T09:01:36Z

govcd/adminvdc.go

+
+// CreateOrgVdc creates a VDC with the given params under the given organization
+// and waits for the asynchronous task to complete.
+// Returns an AdminVdc.


Suggested change

// Returns an AdminVdc.

// Returns an AdminVdc pointer and an error.

dataclouder · 2020-02-05T09:01:54Z

govcd/adminvdc.go

+}
+
+// CreateOrgVdcAsync creates a VDC with the given params under the given organization.
+// Returns an Task.


Suggested change

// Returns an Task.

// Returns a Task and an error

dataclouder · 2020-02-05T09:02:18Z

govcd/adminvdc.go

+}
+
+// createVdc creates a VDC with the given params under the given organization.
+// Returns an Vdc.


Suggested change

// Returns an Vdc.

// Returns a Vdc pointer and an error

govcd/adminvdc_test.go

dataclouder · 2020-02-05T09:16:04Z

govcd/adminvdc_test.go

+		check.Assert(task, Equals, Task{})
+		// From vCD 10+ error changed
+		if vcd.client.Client.APIVCDMaxVersionIs("> 32.0") {
+			check.Assert(err.Error(), Equals, "error retrieving vdc: API Error: 400:  is not a valid unit. Please use MB")


This comparison seems too strict. Can we just check that there is an API error 400? The message may change in future API versions

dataclouder · 2020-02-05T09:18:21Z

govcd/adminvdc_test.go

+	err = adminOrg.Refresh()
+	check.Assert(err, IsNil)
+
+	adminVdc, err := adminOrg.GetAdminVDCByName(vdcConfiguration.Name, false)


Suggested change

adminVdc, err := adminOrg.GetAdminVDCByName(vdcConfiguration.Name, false)

adminVdc, err := adminOrg.GetAdminVDCByName(vdcConfiguration.Name, true)

Using this construct will make the explicit refresh above unnecessary

dataclouder · 2020-02-05T09:19:31Z

govcd/api_vcd_versions.go

@@ -26,6 +26,27 @@ type SupportedVersions struct {
 	VersionInfos `xml:"VersionInfo"`
 }

+// map allows you get vCD version from max supported API version


Suggested change

// map allows you get vCD version from max supported API version

// apiVersionToVcdVersion gets the vCD version from max supported API version

dataclouder · 2020-02-05T09:20:03Z

govcd/api_vcd_versions.go

+	"33.0": "10.0",
+}
+
+// map allows you get max supported API version from vCD version


Suggested change

// map allows you get max supported API version from vCD version

// vcdVersionToApiVersion gets the max supported API version from vCD version

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

govcd/api_vcd_versions.go

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

CODING_GUIDELINES.md

lvirbalas

In general I like the new multi-version support pattern, but some details need to be polished. Thanks!

govcd/adminorg.go

govcd/adminvdc.go

govcd/adminvdc_test.go

types/v56/types.go

CODING_GUIDELINES.md

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

govcd/adminvdc.go

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

lvirbalas · 2020-02-10T12:13:44Z

govcd/adminvdc.go

+
+// CreateOrgVdcAsync creates a VDC with the given params under the given organization.
+// Returns a Task and an error.
+func (adminOrg *AdminOrg) CreateOrgVdcAsync(vdcConfiguration *types.VdcConfiguration) (Task, error) {


Correct me if I'm wrong, but previously CRUD functions for VDC were in adminorg.go. With these changes we have functions for older vCD versions in adminorg.go, but functions for vCD 9.7+ in adminvdc.go. Can we have the same type of functions in a single file, or is there a good reason behind such move?

CC: @dataclouder @Didainius

In early days we try to keep with hierarchy type of storing functions in parent if parent is receiver, but later we chose directly hold functions in files according type. So everything we do new goes to their own file, not in parents. Version 3.0 is good option to cleanup and move functions accordingly.

Is there a reason not to move the touched functions from adminorg.go to adminvdc.go those functions now, so we have them in a single place and have a tidiness from the perspective of CRUD functions? Now it's pretty hard to navigate the code trying to understand where are 9.7+ functions and where are the older ones.

In early days we try to keep with hierarchy type of storing functions in parent if parent is receiver, but later we chose directly hold functions in files according type. So everything we do new goes to their own file, not in parents.

I can move

done

I was actually referring only to the CreateVdc and CreateVdc functions, so they'd all be in a single file with other Create... ones, but I'm OK with this move if everyone else is. Summary:

131 lines moved from adminvdc.go to adminorg.go, functions that moved:

GetAdminVdcByName

GetAdminVDCByHref

GetAdminVDCByName

GetAdminVDCById

GetAdminVDCByNameOrId

CreateVdc

CreateVdc

Commit: 65965d2

CC: @Didainius @dataclouder

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

Didainius

Test suite worked on 9.5 and 10.
I am ok with the concept. A few more comments inline.

CHANGELOG.md

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

CODING_GUIDELINES.md

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

lvirbalas

LGTM! Really great to have a pattern for handling features available only in the new vCD API versions and unlock the potential!

lvirbalas · 2020-02-11T11:55:17Z

CODING_GUIDELINES.md

+
+Functions dealing with different versions should use a matrix structure to identify which calls to run according to the 
+highest API version supported by vCD. An example can be found in adminvdc.go.
+Note: use this pattern for adding new vCD functionality, which is not available in the earliest API version supported 


Nit: please add a new line above this note.

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

dataclouder

Some discussion needed

dataclouder · 2020-02-13T10:19:12Z

CODING_GUIDELINES.md

+}
+
+var vdcVersionedFuncsByVcdVersion = map[string]vdcVersionedFuncs{
+	"vdc9.0":  vdcCrudV90,


Please note that vcdCrudV90 is not defined.

dataclouder · 2020-02-13T10:20:26Z

govcd/adminvdc.go

@@ -23,6 +25,174 @@ func NewAdminVdc(cli *Client) *AdminVdc {
 	}
 }

+// vdcVersionedFuncs holds interface of functions


Suggested change

// vdcVersionedFuncs holds interface of functions

// vdcVersionedFuncs is a generic representation of a VDC creation across multiple versions

@dataclouder "VDC creation" is too specific in this case, because tries to cover all CRUD operations:

What about changing "a VDC creation" to "VDC CRUD operations"? That would read:
// vdcVersionedFuncs is a generic representation of VDC CRUD operations across multiple versions

Yes, it's better. Although we only support creation and update, not all CRUD, since D is missing and the R was explicitly implemented with hidden version choice (see my note under GetAdminVDCByHref ) instead of one function in this structure

dataclouder · 2020-02-13T10:23:09Z

govcd/adminvdc.go

@@ -23,6 +25,174 @@ func NewAdminVdc(cli *Client) *AdminVdc {
 	}
 }

+// vdcVersionedFuncs holds interface of functions
+type vdcVersionedFuncs struct {


vdcVersionedFuncs describes what the type is at a grammatical level, rather than at functional level.
The reject3ed term vdcProducer was showing the purpose of the structure, while the current name only shows that it contains functions.

and what I can do about it?

I still think that a purposeful name such as provider conveys the meaning better than Funcs. How about changing it to vdcCRUD ? At least it suggests some purpose.
cc: @lvirbalas, @Didainius

There was a long discussion about this here:
#285 (comment)

There vdcCrud was proposed too, but at the end we agreed on vdcVersionedFuncs, because:

It has a word "versioned", which makes it instantly clear what the pattern's purpose is

With such name not only CRUD functions could land here if needed

dataclouder · 2020-02-13T10:25:33Z

govcd/adminvdc.go

+	// the latest version, we're limiting the highest used version to the one we support with
+	// the GetSpecificApiVersionOnCondition(...) function. Specifically, the API version 32 returns
+	// two additional fields: IncludeMemoryOverhead and IsElastic for Flex allocation
+	_, err := adminOrg.client.ExecuteRequestWithApiVersion(vdcHref, http.MethodGet,


If possible, I'd like this function to be implemented similarly to the others defined in vdcVersionedFuncs, i.e. one function per version.

Possible, yet it won't bring any value just code repetition

Code repetition is when we repeat the same code in various places. I am saying we should have a GET for 9.0 to 9.5 and another for 9.7+, same as what we did for Create and Update

I don't see any value here until more changes needed. Now I will create same functions for sake of creating and have to make tests, and so on.

dataclouder · 2020-02-13T10:26:01Z

govcd/adminvdc.go

+	return *task, nil
+}
+
+// Creates the vdc and waits for the asynchronous task to complete.


Suggested change

// Creates the vdc and waits for the asynchronous task to complete.

// Creates the VDC and waits for the asynchronous task to complete.

dataclouder · 2020-02-13T10:29:24Z

govcd/adminvdc.go

+	// the GetSpecificApiVersionOnCondition(...) function. Specifically, the API version 32 returns
+	// two additional fields: IncludeMemoryOverhead and IsElastic for Flex allocation
+	_, err := adminVdc.client.ExecuteRequestWithApiVersion(adminVdc.AdminVdc.HREF, http.MethodGet,
+		"", "error refreshing VDC: %s", nil, unmarshalledAdminVdc, adminVdc.client.GetSpecificApiVersionOnCondition(">= 32.0", "32.0"))


Can't we use GetVDCByHref here?

recepients are different. Refresh works with adminVDC and GetVDCByHref is for adminOrg

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

dataclouder

LGTM

POC of versioning

156a05a

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

vmwclabot added the cla-not-required label Jan 22, 2020

vbauzys added 12 commits January 24, 2020 12:45

Add another way of handling versions

24de7e0

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

Removed sudo

03019ce

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

Improvements

6ffd88d

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

Improvements

469bb43

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

Add update ability

008be5e

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

Update comment

0bdd938

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

Update comment

0a4ab92

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

Update comment

1a1da2c

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

Improvements

65ccfe8

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

Improvements

0a21b62

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

Improvements

853d1ba

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

Improvements

7ed3661

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

vbauzys marked this pull request as ready for review January 29, 2020 12:49

vbauzys requested review from dataclouder, lvirbalas and Didainius and removed request for dataclouder and lvirbalas January 29, 2020 12:49

vbauzys self-assigned this Jan 29, 2020

vbauzys requested a review from lvirbalas January 29, 2020 12:49

vbauzys mentioned this pull request Jan 29, 2020

Vdc flex vmware/terraform-provider-vcd#443

Merged

Added changelog

3745d29

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

Didainius requested changes Jan 30, 2020

View reviewed changes

vbauzys added 2 commits January 30, 2020 08:24

Improvements

c8ba6c6

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

Merge branch 'master' into vdc-flex

ab5ed1f

Signed-off-by: Vaidotas Bauzys <[email protected]> # Conflicts: # govcd/org_test.go

make unused var to be used

2dbd012

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

lvirbalas requested changes Feb 4, 2020

View reviewed changes

govcd/adminvdc.go Show resolved Hide resolved

govcd/api_vcd_versions.go Outdated Show resolved Hide resolved

Improvements

09f4bb6

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

dataclouder suggested changes Feb 5, 2020

View reviewed changes

vbauzys added 2 commits February 6, 2020 12:36

Improvements by review

0b58f26

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

Improvements

6309e0d

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

lvirbalas reviewed Feb 7, 2020

View reviewed changes

govcd/api_vcd_versions.go Show resolved Hide resolved

vbauzys added 2 commits February 10, 2020 11:01

Improvements

336d66b

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

Improve comment

b67bd01

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

lvirbalas reviewed Feb 10, 2020

View reviewed changes

CODING_GUIDELINES.md Outdated Show resolved Hide resolved

lvirbalas requested changes Feb 10, 2020

View reviewed changes

Improve comment

52c7844

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

lvirbalas reviewed Feb 10, 2020

View reviewed changes

govcd/adminvdc.go Outdated Show resolved Hide resolved

Improve comment

fae4c4b

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

lvirbalas reviewed Feb 10, 2020

View reviewed changes

Moved admin VDC functions from adminorg to admindvc

65965d2

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

Didainius approved these changes Feb 11, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

vbauzys added 2 commits February 11, 2020 08:06

Improvements

cb07d57

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

Renaming

94c1684

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

lvirbalas requested changes Feb 11, 2020

View reviewed changes

CODING_GUIDELINES.md Outdated Show resolved Hide resolved

CODING_GUIDELINES.md Show resolved Hide resolved

vbauzys added 4 commits February 11, 2020 11:48

Renaming

2ffd512

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

Added guideline

c62d23b

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

Improve guideline

feaabcf

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

Rename vdcfuntion

584906f

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

lvirbalas approved these changes Feb 11, 2020

View reviewed changes

Improve guideline

7ceccf9

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

dataclouder suggested changes Feb 13, 2020

View reviewed changes

vbauzys added 2 commits February 13, 2020 13:47

Improvements

c6497a5

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

Improve comment

a4bbb26

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

dataclouder approved these changes Feb 14, 2020

View reviewed changes

vbauzys merged commit b235a1d into vmware:master Feb 14, 2020

	// Returns an AdminVdc.
	// Returns an AdminVdc pointer and an error.

	adminVdc, err := adminOrg.GetAdminVDCByName(vdcConfiguration.Name, false)
	adminVdc, err := adminOrg.GetAdminVDCByName(vdcConfiguration.Name, true)

	// map allows you get vCD version from max supported API version
	// apiVersionToVcdVersion gets the vCD version from max supported API version

	// map allows you get max supported API version from vCD version
	// vcdVersionToApiVersion gets the max supported API version from vCD version

	// vdcVersionedFuncs holds interface of functions
	// vdcVersionedFuncs is a generic representation of a VDC creation across multiple versions

	// Creates the vdc and waits for the asynchronous task to complete.
	// Creates the VDC and waits for the asynchronous task to complete.

VDC supports Flex #285

VDC supports Flex #285

Conversation

vbauzys commented Jan 22, 2020 • edited Loading

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

vbauzys commented Jan 30, 2020

lvirbalas left a comment

Choose a reason for hiding this comment

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

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

lvirbalas 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

Didainius left a comment

Choose a reason for hiding this comment

lvirbalas left a 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

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

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

vbauzys commented Jan 22, 2020 •

edited

Loading