ceph: move scheme initialization to the same place #8482
Conversation
|
I want to do more tests on this hence the DNM label. |
leseb
force-pushed
the
restrict-manager-cache
branch
3 times, most recently
from
bfdc73d
to
a51b7ee
Compare
pkg/operator/ceph/cr_manager.go
Outdated
| return cache.BuilderWithOptions( | ||
| cache.Options{ | ||
| SelectorsByObject: cache.SelectorsByObject{ | ||
| &cephv1.CephCluster{}: {}, |
There was a problem hiding this comment.
If I read the doc right, this would mean that no fields of the CephCluster and objects with value {} would be cached.
And in others only the namespace field.
https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.9.5/pkg/cache#Options
There was a problem hiding this comment.
Are the reconciler GETs returning empty structs?
Let's initialize the schemes in a single place instead of doing it when each controller initializes. Signed-off-by: Sébastien Han <seb@redhat.com>
leseb
force-pushed
the
restrict-manager-cache
branch
from
a51b7ee
to
2d55e69
Compare
leseb
changed the title
|
In the end, I've decided to go with a small refactor instead. The more I think about this, the more I feel like we will be missing a lot of things and will get unintended behaviors. We might revisit once we have #8400 merged and also with a better resource list of what we manage. (also need to check DS possible impacts...). |
| logger.Info("setting up schemes") | ||
| // Setup Scheme for all resources | ||
| scheme := runtime.NewScheme() | ||
| for _, f := range resourcesSchemeFuncs { |
There was a problem hiding this comment.
What is the benefit of creating the schemes in this central place? It seems better to keep it with each type as it was before. If someone creates a new controller they may not see where to add the scheme here.
There was a problem hiding this comment.
It was useful with the previous version of this patch as the schemes must be defined when building our own cached resources.
I still believe it's nice to have that centralized. If you create a new controller you still need to add the new CRD to pkg/apis/ceph.rook.io/v1/register.go.
At the end, these lines will never be touched again and fewer lines will be added in new controllers.
Thoughts?
There was a problem hiding this comment.
Ok, the functionality is really the same. It's fine either way, we can go with this.
| logger.Info("setting up schemes") | ||
| // Setup Scheme for all resources | ||
| scheme := runtime.NewScheme() | ||
| for _, f := range resourcesSchemeFuncs { |
There was a problem hiding this comment.
Ok, the functionality is really the same. It's fine either way, we can go with this.
Description of your changes:
Let's initialize the schemes in a single place instead of doing it
when each controller initializes.
Signed-off-by: Sébastien Han seb@redhat.com
Which issue is resolved by this Pull Request:
Resolves #
Checklist:
make codegen) has been run to update object specifications, if necessary.