Bump client-go, controller-runtime and related dependencies #5362
Conversation
| } | ||
| if tc.gatewayClassParams != nil { | ||
| client.WithObjects(tc.gatewayClassParams) | ||
| } | ||
| if tc.gateway != nil { | ||
| client.WithObjects(tc.gateway) | ||
| client.WithStatusSubresource(tc.gateway) |
There was a problem hiding this comment.
This was a fun debugging journey. Without this line the tests would complain that status field could not be updated:
gatewaies.gateway.networking.k8s.io "gateway-1" not found
The incorrect pluralization is coming from this funky code. Anyway, adding the line above pre-populates the client with the correct "knowledge" of the object and thus it's pluralization form. I think.
nsimons
added
the
release-note/none-required
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5362 +/- ##
=======================================
Coverage 78.22% 78.23%
=======================================
Files 138 138
Lines 18414 18413 -1
=======================================
Hits 14405 14405
+ Misses 3733 3732 -1
Partials 276 276
|
nsimons
requested review from
skriss and
sunjayBhatia
and removed request for
a team
| func setup(t *testing.T, opts ...interface{}) (cache.ResourceEventHandler, *Contour, func()) { | ||
| // ResourceEventHandlerWrapper wraps the ResourceEventHandler interface from client-go/tools/cache. | ||
| // The OnAdd function has a boolean parameter that we do not need for testing. | ||
| type ResourceEventHandlerWrapper interface { |
There was a problem hiding this comment.
ah unfortunate client-go doesn't have this interface, though they would since they do have this: https://github.com/kubernetes/client-go/blob/4cb373f7caddf872f8dd0e0adbed3d0fe7c6aaf7/tools/cache/controller.go#L234-L238
There was a problem hiding this comment.
@sunjayBhatia I wonder if this kind of abrupt non-backwards compatible change (the addition of isInInitialList bool parameter) in a hugely popular package could be a sign that client-go developers did not expect people to depend on ResourceEventHandler interface and redefine their own OnAdd etc methods directly? It seemed strange decision to break clients just because of new parameter like that. Maybe they expected users only to instantiate ResourceEventHandlerFuncs struct which you point out, and not define their own like Contour does.
There was a problem hiding this comment.
I think the docs on the *Funcs structs imply theyre for usage when you don't need to implement all the methods, so I would expect the authors to know people will rely on the interface itself
there is also *DetailedFuncs here: https://github.com/kubernetes/client-go/blob/4cb373f7caddf872f8dd0e0adbed3d0fe7c6aaf7/tools/cache/controller.go#L264
regardless, unfortunate there was such a change w/o a major version revision
Signed-off-by: Niklas Simons <[email protected]>
Needed for #5214.
Some breaking changes have happened in these dependencies which I have addressed, biggest one being the new boolean parameter in ResourceEventHandler.OnAdd that can be used to know whether we are doing an initial sync or not. I believe we can pass
falsein order to retain old behavior, since we are not using it to know the sync status (yet). For the featuretests I created a simple wrapper to hide this parameter and make the change more manageable.Keeping in draft until controller-runtime 0.15.0 stable version is released.
Signed-off-by: Niklas Simons [email protected]