Allow setting up dependent watches for some resource types, even if dependent watches are disabled#19
Allow setting up dependent watches for some resource types, even if dependent watches are disabled#19
Conversation
This lets the user inject code which produces helm values based on the fetched object itself (unlike `Mapper` which can only see `Values`).
This way the custom code can convert the object to a typed one and work with proper structs rather than a tree of maps from `string` to `interface{}`.
…ing custom status through extensions (#17)
SimonBaeumer
left a comment
SimonBaeumer
left a comment
There was a problem hiding this comment.
Nice change!
Looks good so far, I would only improve the docs and function naming a little bit to be more specific about the new use-case of the boolean parameter.
| } | ||
|
|
||
| // WithExtraDependentWatches is an option that configures whether the reconciler | ||
| // will watch objects of the given kind. These objects will be watched even if |
There was a problem hiding this comment.
| // will watch objects of the given kind. These objects will be watched even if | |
| // will watch objects created by the underlying Helm chart of the given kind. These objects will be watched even if |
|
|
||
| func NewDependentResourceWatcher(c controller.Controller, rm meta.RESTMapper) hook.PostHook { | ||
| func NewDependentResourceWatcher(c controller.Controller, rm meta.RESTMapper, watchReleaseResources bool, extraWatches ...schema.GroupVersionKind) hook.PostHook { |
There was a problem hiding this comment.
I think it is confusing to have watchReleaseResources and extraWatches as parameters in one function and letting the caller decide which one to choose.
Imho adding two functions with different names delegating to a private newDependentResourceWatcher is preferred by me.
// NewDependentResourceWatcherForResources adds depended watches on specific configured resources.
func NewDependentResourceWatcherForResources(c controller.Controller, rm meta.RESTMapper, watches ...schema.GroupVersionKind) hook.PostHook {
return newDependentResourceWatcher(c, rm, false, watches)
}
// NewDependentReleaseResourceWatcher adds watchers to all objects created by the Helm release.
func NewDependentReleaseResourceWatcher(c controller.Controller, rm meta.RESTMapper) hook.PostHook {
newDependentResourceWatcher(c, rm, true)
}
Writing documentation for it is imho more complex and involves mental overhead to understand the concept to why I should add extraWatches if I already added all watched dependencies by setting watchReleaseResources = true.
There was a problem hiding this comment.
But with this change we can no longer separate the "skip dependent watches" and "watch extra resources" parameters. This has to be a single watcher, we cannot just call this function once with watchReleaseResources = true and then with the extra watches; if we ever go back to not skipping dependent watches, this would cause two reconciliations for each update to the resources specified here. Maybe not a concern because we would just drop the extra watches then, but it does complicate the option design substantially.
| if !r.skipDependentWatches || len(r.extraDependentWatches) > 0 { | ||
| r.postHooks = append([]hook.PostHook{internalhook.NewDependentResourceWatcher(c, mgr.GetRESTMapper(), !r.skipDependentWatches, r.extraDependentWatches...)}, r.postHooks...) |
There was a problem hiding this comment.
The renaming of the NewDependetWatches... funcs also removes the need for inverted boolean parameters:
| if !r.skipDependentWatches || len(r.extraDependentWatches) > 0 { | |
| r.postHooks = append([]hook.PostHook{internalhook.NewDependentResourceWatcher(c, mgr.GetRESTMapper(), !r.skipDependentWatches, r.extraDependentWatches...)}, r.postHooks...) | |
| if !r.skipDependentWatches { | |
| r.postHooks = append([]hook.PostHook{internalhook. NewDependentReleaseResourceWatcher(c, mgr.GetRESTMapper())}, r.postHooks...) | |
| } | |
| if len(r.extraDependentWatches) > 0 { | |
| r.postHooks = append([]hook.PostHook{internalhook. NewDependentResourceWatcherForResources(c, mgr.GetRESTMapper(), r.extraDependentWatches...)}, r.postHooks...) | |
| } |
3 participants
This PR adds an
WithExtraDependentWatchesoption, that allows marking certain resources as watched, even if dependent watches are generally disabled.