Add two API in CNS client for GET and POST all NCs #1632

bohuini · 2022-09-27T17:48:44Z

Added two API in CNS client:

GetAllNCsFromCns()

PostAllNCsToCns()

timraymond · 2022-09-27T20:17:31Z

Looks pretty good @bohuini , but you should also add tests in cns/client/client_test.go as well to keep the coverage up.

bohuini · 2022-09-27T20:21:03Z

Sure, thanks @timraymond . I'll add test and publish the PR when done.

timraymond · 2022-10-03T17:44:50Z

@bohuini Could you delete the merge commit and rebase this on master instead? I'd be happy to show you how.

timraymond · 2022-10-03T17:46:46Z

cns/client/client.go

+	return response, nil
+}
+
+func (c *Client) PostAllNCsToCns(ctx context.Context, createNcRequest cns.PostNetworkContainersRequest) error {


Nit: the ToCns part of each method name is just extra words. It's a CNS client, so it's implied that the request is to CNS.

Changed the name.

timraymond · 2022-10-03T17:50:52Z

cns/client/client.go

+	var body bytes.Buffer
+	err := json.NewEncoder(&body).Encode(createNcRequest)
+	if err != nil {
+		return fmt.Errorf("postAllNetworkContainers failed with error: %w", err)


This should be errors.Wrap instead. Also, the message should be what you were trying to do when it failed. In this instance, it should be just something like "encoding request JSON". The reason is that when all of those strings are combined, you get a nice faux stack trace that helps you track it down (and can be found using grep), for example: calling PostAllNCs: encoding request JSON: blah

Good point. Fixed.

timraymond · 2022-10-03T17:52:53Z

cns/client/client.go

+	var response cns.PostNetworkContainersResponse
+	err = json.NewDecoder(resp.Body).Decode(&response)
+	if err != nil || response.Response.ReturnCode != types.Success {
+		return errors.Wrap(err, "decoding Post Network Containers response as JSON")


Suggested change

return errors.Wrap(err, "decoding Post Network Containers response as JSON")

return errors.Wrap(err, "decoding PostNetworkContainersResponse as JSON")

timraymond · 2022-10-03T17:53:39Z

cns/client/client.go

+	var response cns.GetAllNetworkContainersResponse
+	err = json.NewDecoder(resp.Body).Decode(&response)
+	if err != nil || response.Response.ReturnCode != types.Success {
+		return cns.GetAllNetworkContainersResponse{}, errors.Wrap(err, "decoding Get all Network Containers response as JSON")


Suggested change

return cns.GetAllNetworkContainersResponse{}, errors.Wrap(err, "decoding Get all Network Containers response as JSON")

return cns.GetAllNetworkContainersResponse{}, errors.Wrap(err, "decoding GetAllNetworkContainersResponse as JSON")

timraymond · 2022-10-03T17:57:03Z

cns/client/client_test.go

+var (
+	svc              *restserver.HTTPRestService
+	dnsServers       = []string{"8.8.8.8", "8.8.4.4"}
+	errBadRequest    = errors.New("bad request")
+	createNcRequests = []cns.CreateNetworkContainerRequest{
+		{
+			Version:              "12345",
+			NetworkContainerType: "type1",
+			NetworkContainerid:   "nc1",
+			OrchestratorContext:  json.RawMessage("null"),
+		},
+		{
+			Version:              "12345",
+			NetworkContainerType: "blah",
+			NetworkContainerid:   "nc2",
+			OrchestratorContext:  json.RawMessage("null"),
+		},
+	}
+	postAllNcsRequests = cns.PostNetworkContainersRequest{CreateNetworkContainerRequests: createNcRequests}
+)


All of these package variables should either be instantiated directly in the tests themselves or come from a factory function of some kind. Otherwise we end up with gnarly inter-test dependencies like we have in DNC.

timraymond · 2022-10-03T18:02:25Z

cns/client/client_test.go

+	for _, test := range createNCTests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()


This is where there would be a hazard using an uncontrolled package variable for your test cases. Because the package variable is so easy and convenient to reuse, someone will eventually reuse it. If they write to it in a Parallel subtest, then we have a data race in the tests.

Keep the parallel subtests, because I don't think there's any reason why they can't be run in parallel (and fast tests are good). But we need to make sure that the data is sound.

Removed t.Parallel() and the tests works.

@bohuini that's fine too... as I said, I do like using t.Parallel() wherever possible. Unit tests should be blazing fast (less than 10s for the entire suite IMO), and t.Parallel() helps a lot with this. I don't think it'll matter a ton here, so don't feel obligated to add it back. I just wanted to clarify my position.

timraymond · 2022-10-03T18:03:48Z

cns/client/client_test.go

+				}
+
+				for i := 0; i < len(test.expReq.CreateNetworkContainerRequests); i++ {
+					assert.Equal(t, test.expReq.CreateNetworkContainerRequests[i], gotReq.CreateNetworkContainerRequests[i])


Is this stable against different orderings of the elements of the slice? You may have to sort here first...

The order will affect the results. So it needs to be sorted at first.

bohuini · 2022-10-03T21:43:47Z

@timraymond Rebased the branch and simplified the commits

timraymond

Looks great 👍

rbtr · 2022-10-06T19:50:32Z

/azp run

azure-pipelines · 2022-10-06T19:50:57Z