openstack: Fix nil HostManager in openstackContext
#610
Conversation
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 7755661276
💛 - Coveralls |
|
nicely found, thanks @zeeke , as usual! |
pkg/platforms/openstack/openstack.go
Outdated
| @@ -103,7 +104,11 @@ type OSPDeviceInfo struct { | |||
| } | |||
|
|
|||
| func New() OpenstackInterface { | |||
There was a problem hiding this comment.
New should accept hostManager interface IMO.
similarly to what we do with interfaces and their impl in host pkg
There was a problem hiding this comment.
+1 it will also help us if we need to pass a mock for unit-tests
you will only need to add the hostManager creation here
sriov-network-operator/main.go
Line 141 in 5887e75
the other two places where we call the platform NewDefaultPlatformHelper() already have hostHelper created
There was a problem hiding this comment.
In service.go and start.go we create the HostHelper, not the HostManager.
I'm not 100% getting the difference between these actors, but I'd leave the helpers's New function without arguments, and all the other interfaces get created by injecting the dependencies. WDYT?
There was a problem hiding this comment.
and all the other interfaces get created by injecting the dependencies. WDYT?
you will not be able to do so if the test is in a different package. e.g platform_test.
that said, this is not the case ATM. so im fine with either.
we can always change later.
Refactoring in [1] left the openstackContext uninitialized. [1] k8snetworkplumbingwg#553 Signed-off-by: Andrea Panattoni <[email protected]>
zeeke
force-pushed
the
openstack-panic-fix
branch
from
58e0d3c to
1c76c13
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Refactoring in [1] left the openstackContext uninitialized, making the config-daemon crash on OpenStack deployments.
This error has been caught in Openshift downstream CI [2]
[1] #553
[2] openshift/sriov-network-operator#882 (comment)
cc @SchSeba, @EmilienM