-
Notifications
You must be signed in to change notification settings - Fork 114
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
conformance-test: ExcludeTopology field #451
conformance-test: ExcludeTopology field #451
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
2390dd5
to
c74f52a
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
When creating a new test ClientSet, it's useful to log errors to the console. Signed-off-by: Andrea Panattoni <[email protected]>
Pull Request Test Coverage Report for Build 5645670366
💛 - Coveralls |
c74f52a
to
ee19629
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
This looks great! |
ee19629
to
412fd79
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
I added a test case. I'm not 100% happy with asserting the configuration by matching strings, but the alternative (like deserializing the ResourceConfig) would be more complicated. Do you have opinion on that? |
412fd79
to
cd785a8
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
the test looks good.
just small comments
cd785a8
to
328a4ad
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
328a4ad
to
a30be01
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
@SchSeba @Eoghan1232 @adrianchiris Can you please take a look ? |
|
||
intf := unusedSriovDevices[0] | ||
|
||
excludeTopologyTrue := &sriovv1.SriovNetworkNodePolicy{ |
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.
excludeTopologyTrue
, excludeTopologyFalse
can be moved to common place under Context ?
i see they both defined the same in both test cases.
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 moved the declaration under Context. BTW, assignments need the node name and the interface name, so I had to put them under BeforeEach. WDYT?
Overall looks good, one small comment and we can merge IMO |
Add an End2End to validate two different SriovNetworkNodePolicies can't specify different `ExcludeTopology` values for the same resource. Verify excludeTopology field is correctly forwarded to the sriov-device-plugin configuration. Signed-off-by: Andrea Panattoni <[email protected]>
a30be01
to
c3b8137
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Add an End2End to validate two different
SriovNetworkNodePolicies can't specify different
ExcludeTopology
values for the same resource.Needs: