Port DeleteNetworkContainer and SetOrchestratorType from DNC #1454
Conversation
DNC needs to delete network containers using CNS, and it currently does so through raw HTTP requests. This instead makes this an official operation within the CNS client so that it can be used there instead.
The return value from DeleteNetworkContainer was basically unused in DeleteNetworkContainer, since it only contains a CNSResponse type which can only ever be a successful response. Given this, it's sufficient to just return no error as a signifier of success.
DNC uses SetOrchestratorType currently by making straight HTTP requests to the CNS backend. Since we should have one client only, this moves these endpoints into the CNS client so that it can be consumed in DNC.
ramiro-gamarra
left a comment
ramiro-gamarra
left a comment
There was a problem hiding this comment.
One comment for discussion
| // if a non-zero response code was received from CNS, it means something went | ||
| // wrong and it should be surfaced to the caller as an error | ||
| if out.Response.ReturnCode != 0 { | ||
| return errors.New(out.Response.Message) |
There was a problem hiding this comment.
Would a client want to handle errors differently for different error codes? I see other methods doing something similar but if we are going to start consuming this from DNC, I would like to leave this as open for future compatibility and return the actual API contract struct. Probably create new methods for all the methods that we will use from DNC.
There was a problem hiding this comment.
I think it is likely that a client would want to handle errors differently (notably retries for temporary errors). I still think the better way to do it is by having a TemporaryError like I did in the NMAgent client (this is the same idea as Dave Cheney's "don't just check errors, handle them gracefully").
I like this because the knowledge about what constitutes a temporary error (i.e. what do the error codes mean) is in exactly one place: the IsTemporary() method (or equivalent for other classes of error).
In one instance the linter had a false positive on an error that doesn't really need to be wrapped. The other was a good suggestion since it helps readers of the test understand what is going on.
Turns out DNC uses a few more endpoints than it would seem. This adds a CreateNetworkContainer method to encapsulate the /network/createorupdatenetworkcontainer endpoint.
Publishing network containers via CNS is something that DNC does directly through an HTTP client. Given how common this is, it makes sense to adopt this into the CNS client so that DNC can use the client instead.
This was just used in testing and was forgotten.
Everything involving the network should take a context parameter.
The PublishNetworkContainer endpoint returns a wrapping type around the cns.Response. This updates the tests and the endpoint to reflect that.
Unpublishing NCs is accomplished by DNC currently by directly making HTTP calls. This adds that functionality to the client so the client can be used instead.
3 participants
We currently use
/network/deletenetworkcontainerand/network/setorchestratortypein DNC by directly making HTTP requests. There was a small CNS client present to provide access to these two endpoints. This adds these two endpoints to the CNS client we already maintain so that we can just use the "official" one instead in DNC.Notes: