Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48

amberimam · 2020-12-02T14:26:47Z

https://jira-eng-sjc4.cisco.com/jira/browse/NSMNSE-44

…) and BuildVppIfName() functions

This reverts commit 58a1f62.

…sClient() and BuildVppIfName() functions" This reverts commit 61a6061.

…oint stub and add stub unit-tests as well

cmd/vl3-nse/nse_test.go

cmd/vl3-nse/aws_tgw_connect_test.go

cmd/vl3-nse/aws_tgw_connect.go

cmd/vl3-nse/aws_tgw_connect_test.go

CosminNechifor

Nice work. LGTM and I have 2 small suggestions.

cmd/vl3-nse/aws_tgw_connect.go

cmd/vl3-nse/aws_tgw_connect_test.go

cmd/vl3-nse/vl3_connect.go

CosminNechifor · 2020-12-03T10:28:38Z

pkg/nseconfig/config.go

+	Name   		string `yaml:"name"`
+	Labels 		Labels `yaml:"labels"`
+	NseName 	string `yaml:"nseName"` //TODO temporary in order to be able to run examples
+	NseControl 	*NseControl `yaml:"nseControl"`
+	VL3 		VL3 `yaml:"vl3"`
+	AwsTgwEndpoint *AwsTgwEndpoint


Format this file. Also you could add after AwsTgwEndpoint *AwsTgwEndpoint `yaml:"awsTgwEndpoint"`

Sure, what should I format exactly? I am moving the annotations so that they all start in the same column. Should I do anything else?

…) and BuildVppIfName() functions

…oint stub and add stub unit-tests as well

* test: Introduce e2e testing Signed-off-by: Ondrej Fabry <[email protected]> * Update docs for end-2-end tests Signed-off-by: Ondrej Fabry <[email protected]>

…ckend_test.go

…nce I changed it to a var so that it would be easier to override it for unit testing

…) and BuildVppIfName() functions

…oint stub and add stub unit-tests as well

…ckend_test.go

…nce I changed it to a var so that it would be easier to override it for unit testing

tiswanso · 2020-12-04T16:39:53Z

cmd/vl3-nse/vl3_connect.go

@@ -70,6 +70,117 @@ type vL3ConnectComposite struct {
 	nseControlAddr string
 }

+// newVL3ConnectComposite creates a new VL3 composite


not sure what happened/changed here. Is anything supposed to have changed in this file? Even if there are changes please don't move the function as it shows everything as changed in the diffs

ah sure, I'll move it back. I think I misunderstood a review comment from @CosminNechifor . I changed the function to be a var so that I could mock it during unit testing. @CosminNechifor did you just want me to declare the var near the top of the file and leave the function implementation where it is?

tiswanso · 2020-12-04T16:42:10Z

cmd/vl3-nse/service_registry.go

@@ -108,7 +108,7 @@ func (s *serviceRegistry) RegisterWorkload(ctx context.Context, workloadLabels m
 	logrus.Infof("Sending workload register request: %v", serviceWorkload)
 	_, err = s.registryClient.RegisterWorkload(ctx, serviceWorkload)
 	if err != nil {
-		logrus.Errorf("service registration not successful: %w", err)
+		logrus.Errorf("service registration not successful: %v", err)


are these golint related fixes? if there's a lot of these please add these as a separate PR

When I tried running the unit tests I wrote for aws_tgw_connect.go there was a build failure for this file that was preventing my tests from running. Failure happened due to having the %w instead of %v.

Just noting here for exaplanation:
This most likely a forgotten formatting directive that was probably copy-pasted from other fmt.Errorf call. The %w is a special specified for errors, read more here: https://golang.org/pkg/fmt/#Errorf Since this is logrus.Errorf that just prints log and does not create error it is incorrect in this case.

tiswanso · 2020-12-04T17:55:44Z

cmd/vl3-nse/nse_test.go

+	assert.Equal(t, 1, len(*got))
+
+	compositeEndpoints := *got
+	typeofEndpoint := fmt.Sprintf("%T", compositeEndpoints[0])


I don't think this test is actually validating that the AwsTgwConnector endpoint didn't get added. You should loop through the compositeEndpoints to check each element to ensure it's not AwsTgwConnector

Sure, will do.

tiswanso · 2020-12-04T17:57:48Z

cmd/vl3-nse/nse_test.go

+	assert.Equal(t, len(*got), 2)
+
+	compositeEndpoints := *got
+	awsTgwEndpoint := compositeEndpoints[1]


same comment re: looping through the compositeEndpoints.

tiswanso · 2020-12-04T17:58:35Z

cmd/vl3-nse/nse_test.go

+	awsTgwEndpoint := compositeEndpoints[1]
+	typeOfEndpoint := fmt.Sprintf("%T", awsTgwEndpoint)
+
+	assert.Equal(t, "*main.AwsTgwConnector", typeOfEndpoint )


these 2 cases are good candidates to convert to a table driven approach

Sure will change it to a table driven test

tiswanso · 2020-12-04T18:00:55Z

pkg/universal-cnf/vppagent/backend_test.go

@@ -18,6 +18,7 @@ const (
 	dstIpAddrClient = "192.168.22.2"
 	dstIpRouteClient =  "192.168.0.0/16"
 	ifName = "endpoint0"
+	mechanismType = "MEMIF"


seems like the fixes in this file are unrelated to the WCM connector NSE work, right? Are they more appropriate in another PR ?

Sure, I'll move these changes into a separate PR. Thanks for the review Tim!!! :)

tiswanso · 2021-02-18T22:47:50Z

@amberimam -- let's push this content to a branch named feature-aws-tgw in the wwwin-github repo and we can close this PR here.

Amber Imam and others added 6 commits November 18, 2020 18:56

add unit tests for vppagent backend ProcessEndpoint(), ProcessClient(…

61a6061

…) and BuildVppIfName() functions

Clean up imports

58a1f62

Revert "Clean up imports"

781ab4d

This reverts commit 58a1f62.

Revert "add unit tests for vppagent backend ProcessEndpoint(), Proces…

0e3212f

…sClient() and BuildVppIfName() functions" This reverts commit 61a6061.

Implement WCM AWS TGW connector NSE config options and composite endp…

1d7b3e8

…oint stub and add stub unit-tests as well

Merge branch 'master' into nsmnse52

35ec697

amberimam requested review from tiswanso, adodon2go, ondrej-fabry, 91pavan, brecode, snowbldr, shanchunyang0919 and ziyuguo716 December 2, 2020 14:29

vladmoisescu reviewed Dec 2, 2020

View reviewed changes

cmd/vl3-nse/nse_test.go Outdated Show resolved Hide resolved

cmd/vl3-nse/aws_tgw_connect_test.go Outdated Show resolved Hide resolved

vladmoisescu reviewed Dec 2, 2020

View reviewed changes

cmd/vl3-nse/aws_tgw_connect.go Outdated Show resolved Hide resolved

cmd/vl3-nse/aws_tgw_connect_test.go Show resolved Hide resolved

Sort imports and add newlines to files where they were missing

7466939

CosminNechifor reviewed Dec 3, 2020

View reviewed changes

Amber Imam and others added 12 commits December 3, 2020 07:12

add unit tests for vppagent backend ProcessEndpoint(), ProcessClient(…

d9ebb41

…) and BuildVppIfName() functions

Implement WCM AWS TGW connector NSE config options and composite endp…

a051f8f

…oint stub and add stub unit-tests as well

Sort imports and add newlines to files where they were missing

e94b229

test: Introduce e2e testing (cisco-app-networking#35)

d88f34c

* test: Introduce e2e testing Signed-off-by: Ondrej Fabry <[email protected]> * Update docs for end-2-end tests Signed-off-by: Ondrej Fabry <[email protected]>

Merge branch 'nsmnse52' of github.com:amberimam/nsm-nse into nsmnse52

df5bc43

use contants defined at top of file instead of redeclaring vars in ba…

4b85204

…ckend_test.go

format code properly, move newVL3Composite function to top of file si…

40c3caa

…nce I changed it to a var so that it would be easier to override it for unit testing

Merge branch 'master' of https://github.com/cisco-app-networking/nsm-nse

8cc9d40

add unit tests for vppagent backend ProcessEndpoint(), ProcessClient(…

8c7ae21

…) and BuildVppIfName() functions

Implement WCM AWS TGW connector NSE config options and composite endp…

4de87b6

…oint stub and add stub unit-tests as well

Sort imports and add newlines to files where they were missing

2476701

use contants defined at top of file instead of redeclaring vars in ba…

1ac18e8

…ckend_test.go

Amber Imam added 2 commits December 3, 2020 09:26

format code properly, move newVL3Composite function to top of file si…

705bdde

…nce I changed it to a var so that it would be easier to override it for unit testing

Merge branch 'nsmnse52' of github.com:amberimam/nsm-nse into nsmnse52

786f90b

tiswanso reviewed Dec 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48

Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48

amberimam commented Dec 2, 2020

CosminNechifor left a comment

CosminNechifor Dec 3, 2020

amberimam Dec 3, 2020

tiswanso Dec 4, 2020

amberimam Dec 4, 2020

tiswanso Dec 4, 2020

amberimam Dec 4, 2020 •

edited

Loading

ondrej-fabry Dec 8, 2020 •

edited

Loading

tiswanso Dec 4, 2020

amberimam Dec 4, 2020

tiswanso Dec 4, 2020

amberimam Dec 4, 2020

tiswanso Dec 4, 2020

amberimam Dec 4, 2020

tiswanso Dec 4, 2020

amberimam Dec 4, 2020

tiswanso commented Feb 18, 2021

Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48

Are you sure you want to change the base?

Implement WCM AWS TGW connector NSE config options and composite endpoint stub and add stub unit-tests as well #48

Conversation

amberimam commented Dec 2, 2020

CosminNechifor 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

amberimam Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

ondrej-fabry Dec 8, 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

tiswanso commented Feb 18, 2021

amberimam Dec 4, 2020 •

edited

Loading

ondrej-fabry Dec 8, 2020 •

edited

Loading