Skip to content

Conversation

@ashvindeodhar
Copy link
Member

fix: Update CNS to handle updated DNC API in managed mode

This change updates CNS code to consume the updated DNC API urls for RegisterNode and
SyncNodeNcStatus. This change also refactors the calls to DNC in dncclient package.

@ashvindeodhar ashvindeodhar changed the title Fork update managed mode url Update CNS to handle updated DNC API in managed mode Nov 6, 2020
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #725 (aa738f7) into master (13677e6) will increase coverage by 0.09%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #725      +/-   ##
==========================================
+ Coverage   38.93%   39.03%   +0.09%     
==========================================
  Files          83       83              
  Lines       10697    10683      -14     
==========================================
+ Hits         4165     4170       +5     
+ Misses       6034     6006      -28     
- Partials      498      507       +9     

Copy link
Contributor

@ramiro-gamarra ramiro-gamarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments

if sleep {
time.Sleep(acn.FiveSeconds)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is code that was moved, but perhaps it's a good chance to clean it up since it is a bit hard to follow. My main recommendation would be to extract the body of the loop to a function such as:

func registerNode(httpCl *http.Client, url string, body io.Reader) (cns.SetOrchestratorTypeRequest, error) {
	response, err := httpCl.Post(url, acn.JsonContent, body)
	if err != nil {...}
	defer response.Body.Close()

	if response.StatusCode != http.StatusOK {...}

	var req cns.SetOrchestratorTypeRequest
	_ = json.NewDecoder(response.Body).Decode(&req)
	return req, nil
}

which then makes the loop much simpler:

for {
	req, err := registerNode(httpc, registerNodeURL, &body)
	if err != nil {
		logger.Errorf(err.Error())
		time.Sleep(acn.FiveSeconds)
		continue
	}
	httpRestService.SetNodeOrchestrator(&req)
	break
}

To go a step further, I would consider making a proper DNCClient struct, which embeds reusable dependencies such as an http client. The main benefit of this is that the calling code can then use an interface and stub out the actual implementation when writing tests. That can be a later refactor though. See cnsclient in the dnc repo as an example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have updated the logic. I will create the DNCClient struct as a part of next PR where I'll handle the msi in the dncclient.

Copy link
Contributor

@vakalapa vakalapa Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored this section in #680 , along with Ramiro's suggestion, i added ticker logic too

response.Body.Close()
}

return nodeInfoResponse, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to return early. it's easier then to read the happy path of the code without much indentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions
Copy link

This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Feb 11, 2022
@github-actions
Copy link

Pull request closed due to inactivity.

@github-actions github-actions bot closed this Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants