-
Notifications
You must be signed in to change notification settings - Fork 781
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
refactor(windows): win-bridge #617
Conversation
222fe58
to
87758ff
Compare
07a8a55
to
1a06c1e
Compare
looks okay on a quick glance, but let run through this more in depth |
hi ! im gonna try to get some context for this .... |
436a4cd
to
e1f5c37
Compare
|
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.
From my limited knowledge of CNI implementation on windows, it looks good. I don't know how do we verify the netconf part with any sort of integration test? It depends on HCN/HNS interfaces on Windows.
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.
Large change moving a lot of stuff around. Little hard to follow but nothing jumps out so LGTM. Some minor comments/clarifications could help.
if strings.EqualFold(hnsEndpoint.VirtualNetwork, epInfo.NetworkId) { | ||
return nil, fmt.Errorf("HNSEndpoint %s is already existed", epInfo.EndpointName) | ||
} | ||
// remove endpoint if corrupted |
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.
What constitutes the endpoint being corrupted in this case?
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.
I think the endpoint is corrupted if it has the same name but mounted on the other network.
- format function names - add/remove comments - adjust message of error Signed-off-by: thxcode <[email protected]>
- group functions by HNS,HCN Signed-off-by: thxcode <[email protected]>
Signed-off-by: thxcode <[email protected]>
Signed-off-by: thxcode <[email protected]>
cffe767
to
9f802c2
Compare
- support v2 api - unify v1 and v2 api BREAKING CHANGE: - remove `HcnPolicyArgs` field - merge `HcnPolicyArgs` into `Policies` field Signed-off-by: thxcode <[email protected]>
9f802c2
to
4b180a9
Compare
Thanks for all the reviews and work. |
There are some problems with
win-bridge
plugin at present, i.e: unclear error message, unclear function name, cannot apply v2 API in policies...This PR is going to clear/group the function name, comments, and some error messages through two
chore
commits. Then, through tworefactor
commits, some adjustments were made to the process of creating and deleting endpoints, so that the API processing logic of both versions is almost the same. Finally, through a breakingrefactor
commit to unify the two versions of the policy applying processing.I think the impact of this final breaking commit is very small as the v2 API lacks the use under dockershim. Merging
HcnPolicyArgs
intoPolicies
field to unify the endpoint policy configuration in one place. We still keep the same as before under dockershim, but change as below when switching to containerd.