Separate nm status initalization from update #123
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test 4.15-openshift-e2e |
| if err != nil { | ||
| if errUpdate := r.Client.Status().Update(ctx, nm); errUpdate != nil { |
There was a problem hiding this comment.
why do you update the status here?
There was a problem hiding this comment.
Previously it was suggested to move the status update outside the initialization function so this function can be used in unit tests. The function fails in the unit test when the update fails.
But we don't need to update it only when there is an initialization error...
| if err != nil { | ||
| if errUpdate := r.Client.Status().Update(ctx, nm); errUpdate != nil { | ||
| err = fmt.Errorf("errUpdate %w - %w", errUpdate, err) |
There was a problem hiding this comment.
will this result in a useful log line? 🤔
Separation of initMaintenanceStatus logic to only nm status initialization and update is done only afterwards. It is mostly needed for UTs of the initMaintenanceStatus function
razo7
force-pushed
the
separate-nm-status-initalization
branch
from
80b4e31 to
ac116be
Compare
|
/test 4.15-openshift-e2e |
InitMaintenanceStatus initializes the nm CR status only at the first time, when phase is empty, and it returns a bool whether we should update the CR status and an error if it can't be initialized. Without this change an update to CR will happen at the beginning of every reconcile call
razo7
force-pushed
the
separate-nm-status-initalization
branch
from
7366dfa to
b84b512
Compare
Only checking the error is not good enough to verify whether the function can set/initalize the stauts
|
/test 4.15-openshift-e2e |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano, razo7, slintes 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 |
|
/retest |
What this PR does / why we need it:
Take out the nm CR update from the initMaintenanceStatus function so the function can easily be tested (and succeeded) in UTs.
to only nm status initialization and update is done afterward. It is mostly needed for UTs of the initMaintenanceStatus function
Changes made
Separation of initMaintenanceStatus logic to only nm status initialization and update is done only afterwards. It is mostly needed for UTs of the initMaintenanceStatus function
Which issue(s) this PR fixes:
Follow up for comment