Add vApp network functions #290

vbauzys · 2020-02-14T12:15:10Z

Add new function need for handling vApp networks:

Add AddNetwork, AddNetworkAsync
Add AddOrgNetwork, AddOrgNetworkAsync
Add UpdateNetwork, UpdateNetworkAsync
Add UpdateOrgNetwork, UpdateOrgNetworkAsync
Add RemoveNetwork
Add GetUuidFromHref

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

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

lvirbalas

First trivial ask:

The PR subject ("Vapp netw") needs to become a proper sentence with proper words and proper capitalization. Some good examples are here:
hashicorp/terraform-provider-vcd@7a6deb1
hashicorp/terraform-provider-vcd@ae4a431
a453fb9

This is actual to fix now as the last PR got merged with the incomplete PR subject into the log:
hashicorp/terraform-provider-vcd@86fb83f

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

govcd/vapp.go

govcd/vapp_test.go

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

Didainius

First round. I've left many open questions instead of problems yet.

(Changelog confict also waits you :)

Didainius · 2020-02-19T07:05:06Z

govcd/system.go

+
+// Returns the UUID part of an HREF
+// Similar to getBareEntityUuid, but tailored to HREF
+func GetUuidFromHref(href string) (string, error) {


A few questions - do we really need this to be exported? Maybe it could work internally only?
Also - is there any difference between our current private function getUuidFromHref? Can they be wrapped or reused?

This functions used widely in terraform also due that we need get ID from href -> specific vapp network handling. Private function is slight different - regular expression is slight changed.

The only difference between the regular expressions in public and private functions is that the public one allows text after the UUID.
I think that, instead of adding a function that is 99% the same as another, we should add a parameter to match either open ended or closed strings.
Alternatively, the two functions should have a different name, because getUuidFromHref matches an entity UUID (ID at the end), while GetUuidFromHref matches an UUID from an action HREF (ID/action at the end).

Given that the action is discarded, I suggest this interface:

func GetUuidFromHref(href string, idAtEnd bool) (string, error) { searchExpression := `^https://.+/([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})` if idAtEnd { searchExpression +=`$` } else { searchExpression += `.*$` } reGetID := regexp.MustCompile(searchExpression) // [...] }

This way, we use just one function for two needs.

Calls to getUuidFromHref will become GetUuidFromHref(href, true), while calls to GetUuidFromHref will become GetUuidFromHref(href, false)

I'm in favor of a common function as well to avoid duplication.

good idea, change added.

govcd/vapp.go

CHANGELOG.md

govcd/vapp.go

Didainius · 2020-02-19T07:29:59Z

govcd/vapp.go

+		NetworkName: newNetworkSettings.Name,
+		Description: newNetworkSettings.Description,
+		Configuration: &types.NetworkConfiguration{
+			FenceMode:        types.FenceModeNAT,


Is it correct even when isolated network is being created?

govcd/vapp.go

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

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

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

govcd/vapp.go

@@ -786,6 +798,329 @@ func (vapp *VApp) AddIsolatedNetwork(newIsolatedNetworkSettings *VappNetworkSett

 }

+// AddNetwork creates isolated or nat routed(connected to Org VDC network) network for vApp.


This function (and the related Async function) need a better name.
In both AddNetwork and AddOrgNetwork we are passing an OrgVdcNetwork as a parameter, but in one case we are assigning an existing network to the vApp, and in the other we are creating a vApp network.
I think the name in this case should be CreateVappNetwork or AddVappNetwork to avoid confusion

@Didainius what is your vote for naming?

We are creating a new network here so probably CreateVappNetwork.

dataclouder · 2020-02-20T09:15:39Z

govcd/vapp.go

+	return vAppNetworkConfig, nil
+}
+
+// RemoveNetworkAsync asyncronously removes any network (be it isolated or connected to an Org Network) from vApp


Suggested change

// RemoveNetworkAsync asyncronously removes any network (be it isolated or connected to an Org Network) from vApp

// RemoveNetworkAsync asynchronously removes any network (be it isolated or connected to an Org Network) from vApp

Thank you. updated

dataclouder · 2020-02-20T09:16:13Z

govcd/vapp.go

+	return vAppNetworkConfig, nil
+}
+
+// AddNetworkAsync creates asyncronously isolated or nat routed network for vApp. Returns Task or error


Suggested change

// AddNetworkAsync creates asyncronously isolated or nat routed network for vApp. Returns Task or error

// AddNetworkAsync creates asynchronously isolated or nat routed network for vApp. Returns Task or error

Thank you. updated

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

Didainius

Few last comments from me.
Thanks for test fixes

govcd/vapp.go

CHANGELOG.md

Didainius · 2020-02-28T05:33:21Z

govcd/vapp.go

@@ -786,6 +798,329 @@ func (vapp *VApp) AddIsolatedNetwork(newIsolatedNetworkSettings *VappNetworkSett

 }

+// AddNetwork creates isolated or nat routed(connected to Org VDC network) network for vApp.


We are creating a new network here so probably CreateVappNetwork.

govcd/vapp.go

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

Didainius

Thanks for fixes.

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

dataclouder

LGTM

vbauzys added 9 commits February 4, 2020 13:44

Store changes

e74a747

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

Add update capability for network

881b021

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

Add addorgnetwork func

01833d7

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

Added org network firewall and nat support

97ba4a7

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

Refactor vapp code and add test

fae5570

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

Add remove function and tests

66fc029

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

Add changelog, move function

832ae0a

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

Fix

52ef47c

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

Fix for update

643b39e

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

vbauzys requested review from dataclouder, lvirbalas and Didainius and removed request for dataclouder and lvirbalas February 14, 2020 12:15

vbauzys self-assigned this Feb 14, 2020

vbauzys requested a review from lvirbalas February 14, 2020 12:15

vmwclabot added the cla-not-required label Feb 14, 2020

Merge branch 'master' into vapp-netw

0428cc6

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

lvirbalas requested changes Feb 14, 2020

View reviewed changes

vbauzys changed the title ~~Vapp netw~~ Add vApp network functions Feb 18, 2020

Improved err handling

d59870d

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

lvirbalas requested changes Feb 18, 2020

View reviewed changes

Improved comments

0cde86f

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

Didainius reviewed Feb 19, 2020

View reviewed changes

vbauzys added 2 commits February 19, 2020 10:12

Improvements according review

ca9b06b

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

Merge branch 'master' into vapp-netw

bd3aceb

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

dataclouder reviewed Feb 20, 2020

View reviewed changes

vbauzys added 3 commits February 21, 2020 16:23

Fix test issues

03951ad

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

Refactor GetUuidFromHref, removed getUuidFromHref

6374b72

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

Fix comment

d61a959

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

vbauzys mentioned this pull request Feb 24, 2020

Add vApp network resources and data sources vmware/terraform-provider-vcd#455

Merged

vbauzys added 4 commits February 26, 2020 12:50

Improve tests

3e13d44

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

Improve tests

af65317

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

Improve tests

0faced0

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

Add comment

15566bf

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

Didainius reviewed Feb 28, 2020

View reviewed changes

Renamed function AddNetwork -> CreateVappNetwork

288bc13

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

Didainius approved these changes Feb 28, 2020

View reviewed changes

Improve update and add update tests

cf1d88d

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

dataclouder approved these changes Mar 4, 2020

View reviewed changes

vbauzys merged commit 3ba5c93 into vmware:master Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vApp network functions #290

Add vApp network functions #290

vbauzys commented Feb 14, 2020

lvirbalas left a comment

Didainius left a comment

Didainius Feb 19, 2020 •

edited

Loading

vbauzys Feb 19, 2020

dataclouder Feb 20, 2020

lvirbalas Feb 20, 2020

vbauzys Feb 24, 2020

Didainius Feb 19, 2020

vbauzys Feb 24, 2020

dataclouder Feb 20, 2020

vbauzys Feb 24, 2020

Didainius Feb 28, 2020

vbauzys Feb 28, 2020

dataclouder Feb 20, 2020

vbauzys Feb 24, 2020

dataclouder Feb 20, 2020

vbauzys Feb 24, 2020

Didainius left a comment

Didainius Feb 28, 2020

Didainius left a comment

dataclouder left a comment

		@@ -786,6 +798,329 @@ func (vapp VApp) AddIsolatedNetwork(newIsolatedNetworkSettings VappNetworkSett

		}

		// AddNetwork creates isolated or nat routed(connected to Org VDC network) network for vApp.

	// RemoveNetworkAsync asyncronously removes any network (be it isolated or connected to an Org Network) from vApp
	// RemoveNetworkAsync asynchronously removes any network (be it isolated or connected to an Org Network) from vApp

	// AddNetworkAsync creates asyncronously isolated or nat routed network for vApp. Returns Task or error
	// AddNetworkAsync creates asynchronously isolated or nat routed network for vApp. Returns Task or error

Add vApp network functions #290

Add vApp network functions #290

Conversation

vbauzys commented Feb 14, 2020

lvirbalas left a comment

Choose a reason for hiding this comment

Didainius left a comment

Choose a reason for hiding this comment

Didainius Feb 19, 2020 • edited Loading

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

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

dataclouder left a comment

Choose a reason for hiding this comment

Didainius Feb 19, 2020 •

edited

Loading