chore: tidy up the wireserver client and usage #1065
Conversation
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
rbtr
changed the title
ramiro-gamarra
left a comment
ramiro-gamarra
left a comment
There was a problem hiding this comment.
few comments/questions
| // queries the IMDS to get the primary interface info and caches it in the server state | ||
| // before returning the result. | ||
| func (service *HTTPRestService) getPrimaryHostInterface(ctx context.Context) (*wireserver.InterfaceInfo, error) { | ||
| if service.state.primaryInterface == nil { |
There was a problem hiding this comment.
- does lock need to be taken?
- flip condition and return early?
| break | ||
| } | ||
|
|
||
| if !found { |
There was a problem hiding this comment.
is the flag necessary? could just build the InterfaceInfo in the loop and return from there. If the loop is done, there was no primary interface
|
|
||
| ifInfo, err := service.imdsClient.GetPrimaryInterfaceInfoFromMemory() | ||
| var ifInfo *wireserver.InterfaceInfo | ||
| ifInfo, err = service.getPrimaryHostInterface(context.TODO()) |
There was a problem hiding this comment.
curious about this change
There was a problem hiding this comment.
least effort fix to the linter complaining about err shadowing
There was a problem hiding this comment.
erm... something is off if the linter will favor this style over the previous one
| // HostPrimaryIP 10.0.0.4 | ||
| HostPrimaryIP = "10.0.0.4" | ||
| // HostSubnet 10.0.0.0/24 | ||
| HostSubnet = "10.0.0.0/24" |
There was a problem hiding this comment.
consts? otherwise, if they need to be mutated for tests, make them part of the fake?
| // Create CNS object. | ||
| httpRestService, err := restserver.NewHTTPRestService(&config, new(imdsclient.ImdsClient), nmaclient) | ||
|
|
||
| httpRestService, err := restserver.NewHTTPRestService(&config, &wireserver.Client{HTTPClient: &http.Client{}}, nmaclient) |
There was a problem hiding this comment.
DefaultClient can be mutated - I don't want a custom client, I explicitly want a default client
3 participants
Reason for Change:
Strips the
cns/imdsclientpackage of dead code, adds tests, and turns what is left in to a better client.Issue Fixed:
Requirements:
Notes: