-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat: add gate in TrustyAI if ISVC CRD is not in the cluster #1620
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1620 +/- ##
==========================================
- Coverage 20.35% 20.34% -0.01%
==========================================
Files 163 163
Lines 11155 11160 +5
==========================================
Hits 2271 2271
- Misses 8644 8649 +5
Partials 240 240 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
- remove wrong comments in test Signed-off-by: Wen Zhou <[email protected]>
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.
Looks good to me now, thanks!
@@ -55,8 +57,17 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. | |||
handlers.ToNamed(componentApi.TrustyAIInstanceName)), | |||
reconciler.WithPredicates( | |||
component.ForLabel(labels.ODH.Component(LegacyComponentName), labels.True)), | |||
reconciler.WithPredicates( |
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 don't remember all the low level details, but if I recall it properly, if you don't explicit use an Or
, then by default it is an And
, can you verify it works as intended ?
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 think i did that at first, but not triggered reconcile, but let me try again.
the previous intenation i was to have a
OR on "kserve" "model-mesh" then the default "AND" on CRD = isvc, from the test it did not work as well (apart from the missing on TrustyAI itself)
Signed-off-by: Wen Zhou <[email protected]>
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.
One comment typo is optional to fix.
Thanks!
). | ||
reconciler.WithPredicates(predicate.Or( // if TrustyAI CR is changed | ||
component.ForLabel(labels.ODH.Component(LegacyComponentName), labels.True), | ||
predicate.Funcs{ // OR if ISVC from kserve or model-mesh is craeted |
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.
predicate.Funcs{ // OR if ISVC from kserve or model-mesh is craeted | |
predicate.Funcs{ // OR if ISVC from kserve or model-mesh is created |
Signed-off-by: Wen Zhou <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lburgazzoli, MarianMacik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ahub-io#1620) * update: add gate in TrustyAI if ISVC CRD is not in the cluster --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit dc64ee1)
if Trustyai is enabled but cluster does not have ISVC (new cluster, cluster never had serving enabled)
DSC.status.components.trustyai
Description
Due of the ongoing changes in TrustyAI, SR wont be a problem any more, the only dependency is ISVC
this requires trustyai-explainability/trustyai-service-operator#400 and its sync to ODH done first
https://issues.redhat.com/browse/RHOAIENG-19261
How Has This Been Tested?
Screenshot or short clip
Merge criteria