feat: watch Ingress CR updates #1319
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
lburgazzoli and
VaishnaviHire
and removed request for
lphiri and
MarianMacik
|
/retest-required |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## incubation #1319 +/- ##
=============================================
Coverage ? 19.01%
=============================================
Files ? 30
Lines ? 3382
Branches ? 0
=============================================
Hits ? 643
Misses ? 2670
Partials ? 69 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
changes to this file seems to be unrelated
There was a problem hiding this comment.
these are auto generated from some previous commits.
main.go
Outdated
| cacheOptions := cache.Options{ | ||
| Scheme: scheme, | ||
| ByObject: map[client.Object]cache.ByObject{ | ||
| uingress: {}, |
There was a problem hiding this comment.
Not sure about the impact of this, since there's no specific configuration, then I think this line does not have effects on the cache, am I wrong ?
There was a problem hiding this comment.
yep, i forgot to put the selector on it for "cluster" instance
| @@ -635,6 +654,22 @@ func (r *DataScienceClusterReconciler) watchDefaultIngressSecret(ctx context.Con | |||
| return nil | |||
| } | |||
|
|
|||
| func (r *DataScienceClusterReconciler) watchIngressResources(ctx context.Context, a client.Object) []reconcile.Request { | |||
| requestName, err := r.getRequestName(ctx) | |||
There was a problem hiding this comment.
unrelated, but getRequestName seems incorrect as it would return a result even if there's no actual instances of a DSC
There was a problem hiding this comment.
let me make another PR to follow this 0 instance case.
the original code was added 10 months ago.....
| }), | ||
| ). | ||
| Watches( // ingress | ||
| &unstructured.Unstructured{ |
There was a problem hiding this comment.
nit: you can probably import the related OpenShift struct
| @@ -531,6 +532,19 @@ func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr | |||
| }), | |||
| builder.WithPredicates(argoWorkflowCRDPredicates), | |||
| ). | |||
| Watches( // ingresscontroller | |||
There was a problem hiding this comment.
do we need this watch ? it seems the same enqueue logic is performed by the Watches(corev1.Secret) below
There was a problem hiding this comment.
all in all, why we need this IngressController? feel it was no need added there at first
- if user set appsDomain, we need to trigger changes to reflect it - only on update event - only check cluster CR - set in cache - move ingresscontroller to watch than own it Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Description
if user set appsDomain, we need to trigger changes to reflect it
move ingresscontroller to watch than own it
related to change from #1318
https://issues.redhat.com/browse/RHOAIENG-15048
How Has This Been Tested?
Screenshot or short clip
Merge criteria