Avoid duplicated irreconcilable status #226
Conversation
There was a problem hiding this comment.
When install or upgrade fails irreconcilable status is set 2 times
Maybe I'm just confused, but AFAICT this change is not really changing this fact. I think multiple u.UpdateStatus(updater.EnsureCondition(conditions.Irreconcilable(...))) calls are coalesced, so still end up as at most single API UPDATE call. So I don't understand where this 2 times is coming from? Can you please explain this case in more detail?
Adding the new UpdateStatus call in the default case definitely looks right.
However I don't think line 628 did any harm (though it might have been superfluous as long as all of the do* methods also set the irreconcilable condition).
| @@ -651,7 +650,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. | |||
| return ctrl.Result{}, err | |||
| } | |||
| default: | |||
| return ctrl.Result{}, fmt.Errorf("unexpected release state: %s", state) | |||
| err := fmt.Errorf("unexpected release state: %s", state) | |||
There was a problem hiding this comment.
Not sure if this is in scope, but I just realized the inconsistency of whether we set or redefine err (err := rather than err =) here, in the preceding case as well as in the prehook loop might have unintended consequences, given that it affects the defer block for the updater Apply call.
I wonder whether we should take a closer look at this.
Fixes pt. 1 in #224
When install or upgrade fails irreconcilable status is set 2 times, this always lead to a CR update and a new reconciliation cycle.