refactor(status): update status.condition in DSC and DSCI #1196
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
CFSNM and
AjayJagan
zdtsw
force-pushed
the
chore_status_component2
branch
from
32fdb2d
to
ea733ce
Compare
| // type: <component>Ready | ||
| // status: Unknown | ||
| // reason: ReconcileStart | ||
| func SetInitComponentCondition(componentName string, enabled bool) conditionsv1.Condition { |
There was a problem hiding this comment.
All these functions do not set/update anything (VS https://pkg.go.dev/k8s.io/apimachinery/pkg/api/meta#SetStatusCondition which operates on existing list, but still "Set" is not the best name there either) but create a new condition object with different properties. I would call that family of functions "New*".
There was a problem hiding this comment.
I update the name from SetInitComponentCondition() to InitControllerCondition()
and a bunch of others: removed Set in prefix
main idea is:
in controller:
- on DSC/DSCI/FTer condition
| case | to call |
|---|---|
| DSC/DSCI just created | UpdateCondition(oldcondition, InitControllerCondition()) |
| deleting DSC by configmap | UpdateCondition(oldcondition, UnavailableCondition()) |
| DSC exist but no DSCI | UpdateCondition(oldcondition, UnavailableCondition()) |
| DSC/DSCI success reconciled | SetCompleteCondition() |
| FTer success applied | SetCompleteCondition() |
| DSCI failed reconcile | SetErrorCondition(...,CapabilityFailed,...) |
| DSC failed reconcile | SetErrorCondition() |
| FTer failed to create | SetErrorCondition() |
- on component condition (under DSC):
| result | to call |
|---|---|
| success + removed | RemoveComponentCondition() |
| failure | UpdateCondition(oldcondition, reconcileCondition) |
| success + managed | UpdateCondition(oldcondition, reconcileCondition) |
| sepcial on argo | UpdateCondition(oldcondition, ArgoExistCondition()) |
| component just set to managed | UpdateCondition(oldcondition, InitComponentCondition()) |
in each component logic:
| result | return |
|---|---|
| success | SuccessComponentCondition(...) used as reconcileCondition |
| failure | FailedComponentCondition(....) used as reconcileCondition |
There was a problem hiding this comment.
So, why not New* when you just create a structure? It makes intentions clear, while reading "InitComponentCondition" at the first glance I would read is "Initialize ComponentCondition".
There was a problem hiding this comment.
i take your suggestion and renamed it also skipped UpdateCondition on this init creation phase
| func UpdateFailedCondition(componentName string, err error) (conditionsv1.Condition, error) { | ||
| FailedCondition := setFailedComponentCondition(componentName) | ||
| FailedCondition.Message = err.Error() | ||
| return FailedCondition, err |
There was a problem hiding this comment.
What the point to return unchanged err you just passed?
There was a problem hiding this comment.
to convert err into String and set as condition.Message.
There was a problem hiding this comment.
It is the point of the function, but it does not answer my question :)
| } | ||
|
|
||
| // UpdateCondition is top caller in DSC controller to handle different case: init, check, error, successetc. | ||
| func UpdateCondition(conditions *[]conditionsv1.Condition, newCondition conditionsv1.Condition) { |
There was a problem hiding this comment.
Plural then, it updates the first argument (vs the callee, which sets the second argument in the first list)
What is the point of the wrapper? I doubt it brings more clarity.
There was a problem hiding this comment.
could have it directly calling conditionsv1.SetStatusCondition everywhere in DSC
but feel like easier to follow the same pattern as RemoveComponentCondition
maybe not be that useful to have it for now, till we want to add more logic within the function
There was a problem hiding this comment.
So it should have been called UpdateComponentCondition. But I did not change my opinion that both wrappers are misleading. They assume there is some logic related to component behind the call while they just operate on the provided lists. The intention (the list and the provided member of the list) is clear from the call itself without the wrapper.
zdtsw
force-pushed
the
chore_status_component2
branch
from
ea733ce
to
8fa0454
Compare
zdtsw
force-pushed
the
chore_status_component2
branch
from
c4d5f82
to
d5eaa54
Compare
zdtsw
force-pushed
the
chore_status_component2
branch
5 times, most recently
from
aa426be
to
9d21dbf
Compare
for Condition: - create a generic function UpdateFailedCondition() for all components to use, return condition and error - keep status types to only "avaiable" "progressing" degraded" and "reconcilesuccess" - status:ReconcileComplete is renamed to ReconcileSuccess - status:upgradeable is removed for pipeline: - move SetExistingArgoCondition from pipeline to status - remove duplicated update on DSC, since error already catch in pipeline and UpdateFailedCondition() is called for Phase: Created, Ready, Error, deleting - set phase to Error if Argo CRD exist - remove Progressing to use Created instead when DSCI CR just created chore: - fix typo and some missing customized error message - fix Message when created - move static reason,message into status package Signed-off-by: Wen Zhou <[email protected]>
zdtsw
force-pushed
the
chore_status_component2
branch
from
9d21dbf
to
abf131e
Compare
Description
re-wrok on #977
feat(status): update status.condition in DSC and DSCI
for Condition:
for Phase: Created, Ready, Error, deleting
chore:
How Has This Been Tested?
Screenshot or short clip
Merge criteria