-
Notifications
You must be signed in to change notification settings - Fork 264
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
Add unit tests to ncproxy #1143
Add unit tests to ncproxy #1143
Conversation
if _, ok := err.(hcn.NetworkNotFoundError); ok { | ||
return nil, status.Errorf(codes.NotFound, "no network/switch with name `%s` found", req.SwitchName) | ||
policies := []hcn.NetworkPolicy{} | ||
if req.SwitchName != "" { |
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 added this change to allow us to make other network types than just transparent as was the previous limitation. This is also needed for the tests since we can't make transparent networks without an adapter.
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.
That's fine, I didn't like how I made this required to start with.
199d095
to
c63024f
Compare
c63024f
to
173bb6a
Compare
4d5976c
to
8fd2e14
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.
lgtm
@@ -35,4 +35,7 @@ const ( | |||
|
|||
// V20H2 corresponds to Windows Server 20H2 (semi-annual channel). | |||
V20H2 = 19042 | |||
|
|||
// V21H1 corresponds to Windows Server 21H1 (semi-annual channel). | |||
V21H1 = 19043 |
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.
Some of the things here, like this, feel like they'd be better off in their own commit.
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 added a comment to the test to indicate why it blocks on this version
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.
ty
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.
Please move the osversion package change into its own commit. Testing changes lgtm
* Add mocked grpc and ttrpc services for ncproxy testing Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
8fd2e14
to
89d75bf
Compare
Related work items: microsoft#1062, microsoft#1087, microsoft#1089, microsoft#1095, microsoft#1104, microsoft#1112, microsoft#1117, microsoft#1118, microsoft#1125, microsoft#1137, microsoft#1139, microsoft#1140, microsoft#1141, microsoft#1142, microsoft#1143, microsoft#1145, microsoft#1146, microsoft#1150, microsoft#1151, microsoft#1153, microsoft#1154, microsoft#1155, microsoft#1156, microsoft#1157, microsoft#1158, microsoft#1159, microsoft#1161, microsoft#1162, microsoft#1163, microsoft#1164, microsoft#1165, microsoft#1166, microsoft#1167, microsoft#1168, microsoft#1169, microsoft#1171, microsoft#1172, microsoft#1173, microsoft#1174, microsoft#1178
Signed-off-by: Kathryn Baldauf kabaldau@microsoft.com