WIP: Refactor reconcilor so each component is returning status #765
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 |
anishasthana
force-pushed
the
component_readiness
branch
from
6bb520f to
b485c5d
Compare
|
Change to PR detected. A new PR build was completed. |
anishasthana
force-pushed
the
component_readiness
branch
from
b485c5d to
bf56f28
Compare
|
Change to PR detected. A new PR build was completed. |
Signed-off-by: Anish Asthana <[email protected]>
anishasthana
force-pushed
the
component_readiness
branch
from
bf56f28 to
54ea85e
Compare
|
Change to PR detected. A new PR build was completed. |
| dspa *dspav1.DataSciencePipelinesApplication, setStatus func(metav1.Condition), | ||
| log logr.Logger) { | ||
| condition, err := r.evaluateCondition(ctx, dspa, resourceName, conditionType) | ||
| condition.Message = statusMessage |
There was a problem hiding this comment.
I'm not sure if overwriting the Message here is the right call. The reason I'm wondering that is that https://github.com/opendatahub-io/data-science-pipelines-operator/blob/main/controllers/dspipeline_controller.go#L406 has a number of instances in which it is setting the Message field itself. Thoughts @hbelmiro ?
There was a problem hiding this comment.
I actually proposed replacing
r.setStatus(ctx, params.MlmdProxyDefaultResourceName, config.MLMDProxyReady, statusMessage, dspa,
dspaStatus.SetMLMDProxyStatus, log)with
condition, err := r.evaluateCondition(ctx, dspa, params.MlmdProxyDefaultResourceName, config.MLMDProxyReady)
condition.Message = status
if err != nil {
r.setStatusAsNotReady(config.MLMDProxyReady, err, dspaStatus.SetMLMDProxyStatus)
return ctrl.Result{}, err
}
dspaStatus.SetMLMDProxyStatus(condition)here.
Related issue: https://issues.redhat.com/browse/RHOAIENG-13321
I took a stab at doing what I think the underlying issue is asking for. @hbelmiro let me know if I'm on the right track!
I haven't gone through and updated all the dependent functions yet since I don't want to go down the stack if my starting point is completely wrong.
Related: #683 (comment)