-
Notifications
You must be signed in to change notification settings - Fork 259
Expose GET NC list API in CNS client #3449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4767364 to
5548caa
Compare
|
/azp run Azure Container Networking PR |
Supported commands
See additional documentation. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- cns/restserver/restserver.go: Evaluated as low risk
- nmagent/responses.go: Evaluated as low risk
Comments suppressed due to low confidence (2)
cns/restserver/api.go:1336
- Use respondJSON instead of common.Encode for consistency: respondJSON(w, http.StatusOK, NCListResponse)
serviceErr := common.Encode(w, &NCListResponse)
cns/restserver/api.go:1308
- Initialize returnCode with a default value, e.g., returnCode = types.Success
var returnCode types.ResponseCode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- cns/restserver/restserver.go: Evaluated as low risk
- nmagent/responses.go: Evaluated as low risk
Comments suppressed due to low confidence (2)
cns/restserver/api_test.go:1327
- [nitpick] The test case should assert based on the expected behavior of the API. If the list is expected to contain items, adjust the assertion accordingly.
require.Empty(t, nmAgentNCListResponse.NCList)
cns/api.go:31
- [nitpick] The constant name NMAgentGetNCListAPIPath should be renamed to GetNCListPath to match the existing naming convention.
NMAgentGetNCListAPIPath = "/nclist"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
adf3752 to
ec1b5aa
Compare
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
9cadc8b to
4109319
Compare
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CNS log when DNC calls this endpoint: |
* initial changes to expose getnclist API in cns client * add a test * linter fix * linter fix * linter fix * change return message to provide a default value * address comments * linter fix * address comments
Reason for Change:
Expose GET NC list API in CNS client. DNC will call this endpoint to fetch the list of NCs programmed on the node.
In the backend, CNS calls NMAgent's http://%s/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/api-version/%s endpoint
Requirements: