update: remove cache for deployment in model reg namespace #1270
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
zdtsw
requested review from
dhirajsb and
VaishnaviHire
and removed request for
jackdelahunt
| const DefaultModelRegistryCert = "default-modelregistry-cert" | ||
| const ( | ||
| DefaultModelRegistryCert = "default-modelregistry-cert" | ||
| ModelRegSelector = "model-registry" |
There was a problem hiding this comment.
can we use component name?
There was a problem hiding this comment.
thought to use Component name at first, but then it uses model-registry-operator also move type var to const.
if that is fine, i can do the change.
There was a problem hiding this comment.
I do not have a strong opinion, and yes, '-operator' confused me as well. But on the other hand, we have pretty defined components, and the label is designed for components, so I'm biased a bit to reuse it. @lburgazzoli ?
There was a problem hiding this comment.
we could have use model-registry-operator maybe it is easier, since all other ModelReg resource have this already used.
tbh, i feel the value for ComponentName should be model-registry not model-registry-operator at first.
There was a problem hiding this comment.
i feel the value for ComponentName should be
model-registrynotmodel-registry-operatorat first.
Yep.
main.go
Outdated
| &appsv1.Deployment{}: {Namespaces: deploymentCache}, | ||
| &appsv1.Deployment{}: { | ||
| Namespaces: deploymentCache, | ||
| Label: pkglables.Set{labels.ODH.Component(modelregistry.ModelRegSelector): "true"}.AsSelector(), |
There was a problem hiding this comment.
not sure I get the rationale here, are we only expected to cache model registry's deployments ?
There was a problem hiding this comment.
you are right. need to move the selector intoe deploymentCache, otherwise, it will only get the modelreg deployment and skip all others
zdtsw
changed the title
- Operator does not need to cache deployment in this namespace - it is up to model reg operator for resoruces created there Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## incubation #1270 +/- ##
=============================================
Coverage ? 18.96%
=============================================
Files ? 30
Lines ? 3390
Branches ? 0
=============================================
Hits ? 643
Misses ? 2678
Partials ? 69 ☔ View full report in Codecov by Sentry. |
ODH operator no need to cache deployment in model reg namespace, as we only care about the applicationnamepsace and a bunch of other for monitorings.
it was a mistake in the past to add modelreg namespace into the cache for deployment resourcec.
this PR is to remove that piece of logic + move the comments about do not label model reg namespace in better place.
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria