-
Notifications
You must be signed in to change notification settings - Fork 827
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
fix(deps): Upgrade sigs.k8s.io/controller-runtime to 0.17.4 #5562
Conversation
This is required as part of a package upgrade
@@ -46,7 +45,7 @@ func TestAPIs(t *testing.T) { | |||
RunSpecsWithDefaultAndCustomReporters( | |||
t, | |||
"Controller Suite", | |||
[]Reporter{printer.NewlineReporter{}}, |
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 library printer
doesnt exist anymore in controller-runtime
and I dont think it is required for testing.
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.
appreciate the comment, helpful to know why the change was needed!
@@ -63,16 +66,16 @@ func main() { | |||
logger := zap.New(zap.UseFlagOptions(&opts)) | |||
ctrl.SetLogger(logger) | |||
|
|||
watchNamespace := namespace | |||
watchNamespaceConfig := map[string]cache.Config{namespace: {}} |
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.
description of the new api is here
@@ -213,7 +212,7 @@ func (r *SeldonRuntimeReconciler) updateStatus(seldonRuntime *mlopsv1alpha1.Seld | |||
} | |||
|
|||
// Find SeldonRuntimes that reference the changes SeldonConfig | |||
func (r *SeldonRuntimeReconciler) mapSeldonRuntimesFromSeldonConfig(obj client.Object) []reconcile.Request { | |||
func (r *SeldonRuntimeReconciler) mapSeldonRuntimesFromSeldonConfig(_ context.Context, obj client.Object) []reconcile.Request { | |||
logger := log.FromContext(context.Background()).WithName("mapSeldonRuntimesFromSeldonConfig") |
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.
would it make sense to pass the (currently unnamed) Context
function argument here?
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 caller doesnt have a Context
though as far as I can tell, I will add a TODO in the code to look at it more deeply.
@@ -184,7 +183,7 @@ func (r *ServerReconciler) updateStatus(server *mlopsv1alpha1.Server) error { | |||
} | |||
|
|||
// Find Servers that need reconcilliation from a change to a given ServerConfig | |||
func (r *ServerReconciler) mapServerFromServerConfig(obj client.Object) []reconcile.Request { | |||
func (r *ServerReconciler) mapServerFromServerConfig(_ context.Context, obj client.Object) []reconcile.Request { | |||
logger := log.FromContext(context.Background()).WithName("mapServerFromServerConfig") |
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.
similar context question as above
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 have added a TODO in the code as per the answer in the previous review 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, with just one comment/question
What this PR does / why we need it:
This PR upgrades
controller-runtime
fromv0.11.2
tov0.17.4
(~2 years difference). Note that there is a newer version of this packagev0.18.1
but it requires go1.22
and we are currently at go1.21
.It required changes as the api has moved since then:
ctrl.NewManager
context
parameter)Which issue(s) this PR fixes:
Fixes INFRA-680 (internal)
TODO: