Conversation
porridge
left a comment
porridge
left a comment
There was a problem hiding this comment.
Mostly some nitpicks. I also think we should introduce new Reasons, but if you have very strong reasons to disagree, I won't fight them.
Also it would be good to provide some guidance on why someone should be using a hook rather than an extension (I don't see any reason, and perhaps we should suggest deprecating them).
pkg/reconciler/reconciler.go
Outdated
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "github.com/joelanford/helm-operator/pkg/extensions" |
There was a problem hiding this comment.
Please move to below line 48.
| for _, ext := range r.preExtensions { | ||
| if err := ext(ctx, obj, r.log); err != nil { | ||
| u.UpdateStatus( | ||
| updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), |
There was a problem hiding this comment.
If there are reasons as detailed as ReasonErrorGettingClient or ReasonErrorGettingValues then I think extensions deserve their own ReasonExtensionError, or perhaps even, a ReasonPreExtensionError and ReasonPostExtensionError, rather than piggy-back on ReasonReconcileError.
There was a problem hiding this comment.
I would rather not do so, because I do not like these reasons to begin with. They expose implementation details. I would much prefer to actually let users specify their own reasons to be set in cases of errors. Eventually, I would like to get rid of all Helm-operator specific reasons.
ReasonReconcileError seems good enough because the extensions implement reconciliation logic themselves and thus are part of the reconciliation process.
Co-authored-by: Marcin Owsiany <marcin@stackrox.com>
Co-authored-by: Marcin Owsiany <marcin@stackrox.com>
misberner
left a comment
misberner
left a comment
There was a problem hiding this comment.
Also it would be good to provide some guidance on why someone should be using a hook rather than an extension (I don't see any reason, and perhaps we should suggest deprecating them).
Added a comment to prefer extensions over hooks unless there is a reason that the respective logic needs to access the translated values (this is not possible with extensions, therefore I did not mark hooks as deprecated). Apart from that, I think deprecation decisions should be done in sync with upstream.
pkg/reconciler/reconciler.go
Outdated
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "github.com/joelanford/helm-operator/pkg/extensions" |
3 participants
This allows for custom reconciliation extensions before and after doing the main Helm work.
Why not hooks?