-
Notifications
You must be signed in to change notification settings - Fork 65
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
SANDBOX-560 update controller-runtime to v0.15 #1057
base: master
Are you sure you want to change the base?
Conversation
… into runtime_15_host
controller, cl := newController(t, ds, toolchainConfig, &toolchainv1alpha1.Notification{}) | ||
// Add notification to fakeclient - update failing because notification status not found as subresource |
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.
but this would mean that you cannot call status update on kinds that were not added as part of the init step of the fake client. This looks a bit weird and like a bug to me. Is these being tracked somewhere or is that an expected feature?
Anyway, you can move adding the empty &toolchainv1alpha1.Notification{}
object into the newController
function and do it for all tests automatically.
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.
This is an expected feature with the addition of SubResource - kubernetes-sigs/controller-runtime#2259
Thanks, added in newController in 6463c77
// Add notification to initObjs - so that can be added as subResource | ||
initObjs = append(initObjs, &toolchainv1alpha1.Notification{}) |
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.
Could you elaborate on this? And Notification is not a sub resource, is it?
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.
my understanding is that all resources with status should be added to fakeclient with withStatusSubresource
. Then the functions update and status().update bifurcate in a way that update doesn't touch status and status().update only updates status.
here I'm adding notification in the initObjs and NewFakeClient
adds those initObjs as StatusSubresource
this is the PR that introduces that change in controller-runtime kubernetes-sigs/controller-runtime#2259
does that help?
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.
So, I took a glance at it. The fake client newly checks if there is GVK registered for the given sub-resource before it tries to update it.
https://github.com/kubernetes-sigs/controller-runtime/blob/735b6073bb253c0449bfcf6641855dcf2118bb15/pkg/client/fake/client.go#L413
But default, the client registers GVKs for hard-coded list of resources:
https://github.com/kubernetes-sigs/controller-runtime/blob/735b6073bb253c0449bfcf6641855dcf2118bb15/pkg/client/fake/client.go#L1240-L1274
and for those that were added via method WithStatusSubresource
https://github.com/kubernetes-sigs/controller-runtime/blob/release-0.15/pkg/client/fake/client.go#L207-L210
which is something Kanika does in her toolchain-common PR for all objects included provided as init object here:
https://github.com/codeready-toolchain/toolchain-common/pull/376/files#diff-1e288d578a8b0f65d3d7f6895c7574365de50afcfe1fd83de1fa2c32819c99e0R26
and because the Notification CR wasn't provided as an init object, then it was registered as containing a sub-resource.
TBH, there is a room for an improvement in our implementation of the fake client. I'll leave a comment there.
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.
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.
Looks good!
|
@@ -460,7 +460,7 @@ func addMemberClusters(mgr ctrl.Manager, cl runtimeclient.Client, namespace stri | |||
// for some resources like SpaceRequest/SpaceBindingRequest we need the cache to be clustered scoped | |||
// because those resources are in user namespaces and not member operator namespace. | |||
if namespacedCache { | |||
options.Namespace = memberConfig.OperatorNamespace | |||
options.Cache.Namespaces = []string{memberConfig.OperatorNamespace} |
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.
SA1019: options.Namespace is deprecated: Use Cache.Namespaces instead. (staticcheck)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, ranakan19 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Updates k8s dependencies to 0.27 and controller-runtime to v0.15.
With the update to controller-runtime v0.15 (changes in release detailed here), this PR has following changes to combat the breaking changes:
Changes wrt - https://issues.redhat.com/browse/SANDBOX-560
DO NOT MERGE (All the PRs related to version updates need to be merged at once, but these PR are for early feedback so that there are not a lot of changes to review at once)