-
Notifications
You must be signed in to change notification settings - Fork 275
Add unit tests to ncproxy #1143
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
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
anmaxvl
left a comment
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
| 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
dcantah
left a comment
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 <[email protected]>
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 [email protected]