✨ remove unnecessary flag, optimize catalog watch handler, stop watching non-existent unpack pods#941
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 79.34% 80.41% +1.06%
==========================================
Files 16 16
Lines 1104 1103 -1
==========================================
+ Hits 876 887 +11
+ Misses 158 150 -8
+ Partials 70 66 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return true | ||
| }, | ||
| }). | ||
| Watches(&corev1.Pod{}, mapOwneeToOwnerHandler(mgr.GetClient(), mgr.GetLogger(), &ocv1alpha1.ClusterExtension{})). |
There was a problem hiding this comment.
Since we're on it - we should also be able to remove the rbac to watch pods:
operator-controller/internal/controllers/clusterextension_controller.go
Lines 105 to 107 in cb63023
If not in this PR, it can be done in follow up too
There was a problem hiding this comment.
Pushed a change. I removed the pods stuff, and changed the configmaps RBAC (what was that for?) to secrets since the helm stuff needs to manage release secrets.
It's all sorta moot though because:
- We give ourselves
*/*/*in the next line. - We'll eventually drop the secrets permission here because we'll expect the SA to have the necessary permission to handle their own release secret.
There was a problem hiding this comment.
and changed the configmaps RBAC (what was that for?)
I think it may have gotten ported over from Extension, where Kapp needed CRUD permissions for configmaps.
joelanford
force-pushed
the
random-cleanup
branch
2 times, most recently
from
186c4ce to
64420af
Compare
|
Looks like 'make verify' or equivalent is needed. |
openshift-ci
bot
added
the
do-not-merge/hold
|
Oops forgot to re-gen RBAC. Will do! |
joelanford
force-pushed
the
random-cleanup
branch
from
64420af to
e0b00b4
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
joelanford
force-pushed
the
random-cleanup
branch
2 times, most recently
from
e437d99 to
1749fb1
Compare
- remove unnecessary flag - optimize catalog watch handler - fix finalizers (also stop using rukpak-based finalizers and keys) - Add some reminder TODO comments about more improvements (need to convert to issues) Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
joelanford
force-pushed
the
random-cleanup
branch
from
1749fb1 to
3b172c3
Compare
| ActionClientGetter: helmClientGetter, | ||
| Unpacker: unpacker, | ||
| InstalledBundleGetter: mockInstalledBundleGetter, | ||
| Finalizers: crfinalizer.NewFinalizers(), |
There was a problem hiding this comment.
It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can. We should discuss further, but off the top of my head, I wonder if we can use the exact same reconciler as is configured in main.go.
- For catalogd we can use httptest to setup a mock catalogd server and use the real ClusterExtension http client.
- An image registry is an http server. I wonder if there is an in-process implementation that would be easy to run for test purposes.
- Use
t.TempDir()subdirectories for any necessary local storage and caching that happens during reconcile. - For the kubernetes cluster, we use envtest.
If we had that setup, we wouldn't actually need to configure the ClusterExtension reconciler any differently.
There was a problem hiding this comment.
More immediately, we should have unit tests that make sure finalizers are actually being handled properly during reconcile.
There was a problem hiding this comment.
It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can.
This is definitely up for discussion and needs separate set of effort. The whole point of these unit tests is to make sure that the reconciler code path traversed during various instances is as expected. Which is why we ensure that the mock unpacker returns quickly as expected. The code coverage for unpacking and storage has separate unit tests (currently in rukpak).
I'm not sure if we immediately need to focus on this - rather if possible try finishing up #879 as immediately as possible (even if it requires us to mock some of the components) to ensure we don't skip any nuances in the reconciler code.
| ActionClientGetter: helmClientGetter, | ||
| Unpacker: unpacker, | ||
| InstalledBundleGetter: mockInstalledBundleGetter, | ||
| Finalizers: crfinalizer.NewFinalizers(), |
There was a problem hiding this comment.
It's a bit disconcerting to me that so much of the ClusterExtensionReconciler can be constructed so differently than the real one can.
This is definitely up for discussion and needs separate set of effort. The whole point of these unit tests is to make sure that the reconciler code path traversed during various instances is as expected. Which is why we ensure that the mock unpacker returns quickly as expected. The code coverage for unpacking and storage has separate unit tests (currently in rukpak).
I'm not sure if we immediately need to focus on this - rather if possible try finishing up #879 as immediately as possible (even if it requires us to mock some of the components) to ensure we don't skip any nuances in the reconciler code.
3 participants
Description
Reviewer Checklist