-
Notifications
You must be signed in to change notification settings - Fork 242
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
Expose GET NC list API in CNS client #3449
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:
|
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: