chore(controllers): Use req.NamespacedName directly

[Baarsgaard]

This removes all the places we store a pointer to the scheme unnecessarily and refactors the controllers to use the req.NamespacedName when fetching the resource in the beginning of all Reconcile functions.

[weisdd]

Not like I perfectly know the internals, but, to me, the change around removal of the scheme does not look right. Even though it's the same across resources, it still needs to be properly propagated from the same instance: scheme = runtime.NewScheme() -> .AddToScheme calls in init() -> Reconcilers.

Here, the Scheme() gets inherited from Client interface, where it's defined as Scheme() *runtime.Scheme meaning it does not appear to be set to the real scheme where all our custom APIs are registered.

And having Scheme *runtime.Scheme as part of any reconciler definition is a common pattern anyway.

So, without extra comments from your side explaining why you think the change is safe, I feel like the change should not be made.

[Baarsgaard]

I went digging through the implementation of the Manager struct to triple-check this.

We init the scheme exactly as you wrote above, but we never instantiate an API client.
Instead we rely on the Scheme in managerOptions

grafana-operator/main.go

Lines 185 to 189 in 129224e

	mgrOptions := ctrl.Options{
	Scheme: scheme,
	Metrics: metricsserver.Options{BindAddress: metricsAddr},
	WebhookServer: webhook.NewServer(webhook.Options{Port: 9443}),
	HealthProbeBindAddress: probeAddr,

mgr is created by calling NewManager() with restConfig and mgrOptions which create the it cluster.Cluster.

grafana-operator/main.go

Lines 259 to 263 in 129224e

	mgr, err := ctrl.NewManager(restConfig, mgrOptions)
	if err != nil {
	setupLog.Error(err, "unable to create new manager")
	os.Exit(1) //nolint
	}

The NewManager function is an alias of Manager.New
https://github.com/kubernetes-sigs/controller-runtime/blob/71f7db556ca57ce7ea6563f77d739f0d2a54233a/alias.go#L114

We see manager.New instantiating the internal cluster:
https://github.com/kubernetes-sigs/controller-runtime/blob/71f7db556ca57ce7ea6563f77d739f0d2a54233a/pkg/manager/manager.go#L328-L340

Cluster.New creating a client from the original mgrOptions.
https://github.com/kubernetes-sigs/controller-runtime/blob/71f7db556ca57ce7ea6563f77d739f0d2a54233a/pkg/cluster/cluster.go#L192-L226

Which define the GetClient and GetScheme functions
https://github.com/kubernetes-sigs/controller-runtime/blob/71f7db556ca57ce7ea6563f77d739f0d2a54233a/pkg/manager/internal.go#L234-L248

Used in our main.go to build and register the controllers:

grafana-operator/main.go

Lines 265 to 274 in 129224e

	// Register controllers
	if err = (&controllers.GrafanaReconciler{
	Client: mgr.GetClient(),
	Scheme: mgr.GetScheme(),
	IsOpenShift: isOpenShift,
	ClusterDomain: clusterDomain,
	}).SetupWithManager(mgr); err != nil {
	setupLog.Error(err, "unable to create controller", "controller", "Grafana")
	os.Exit(1)
	}

TLDR:
We instantiate a mgr struct with an options struct containing the built scheme.
During that instantiation, multiple API clients are built using the restConfig and Scheme which we then use to build our controllers.
Using client.GetScheme() returns the original scheme from the mgrOptions as the clients were instantiated with that Scheme originally.

[Baarsgaard]

The above comment does not address whether this is a good change.
It hides the proliferation of Scheme which may not be ideal.

I'm not against dropping the commit with the Scheme changes if we decide it's not worth it.

[weisdd]

Thanks for the explanation!

Personally, I would rather favour leaving the existing code around Schema as it matches things described in the kubebuilder book, operator-sdk scaffolding, and other places. If other maintainers think the change should be accepted, I'm OK with that, so let's just wait for others to vote on this :)

[theSuess]

We talked about this in the weekly call and decided to keep the scheme. The NamespacedName stuff is good to merge though

[Baarsgaard]

Sounds good, I'll drop the commit later today.