Adjust static "helm-controller" name references to use controllerName #260
Conversation
Signed-off-by: Dan Pock <[email protected]>
mallardduck
force-pushed
the
adjust-lock-for-controller-name
branch
from
7cb703b to
25e2c26
Compare
| @@ -106,7 +108,8 @@ func Register(ctx context.Context, systemNamespace, controllerName string, cfg c | |||
| klog.Infof("Starting helm controller in namespace %s", systemNamespace) | |||
| } | |||
|
|
|||
| leader.RunOrDie(ctx, systemNamespace, "helm-controller-lock", appCtx.K8s, func(ctx context.Context) { | |||
| controllerLockName := controllerName + "-lock" | |||
| leader.RunOrDie(ctx, systemNamespace, controllerLockName, appCtx.K8s, func(ctx context.Context) { | |||
There was a problem hiding this comment.
I suspect that if systemNamespace is blank here, the lock will actually go into the default namespace? But we should validate that, and perhaps allow configuring the lock namespace separate from the watched namespace?
There was a problem hiding this comment.
Yes, that is correct - if systemNamespace is blank the underlying leader.RunOrDie will try to use kube-system (which is the default in this case).
perhaps allow configuring the lock namespace separate from the watched namespace?
I'm open to that, I can see some use-cases where that could be useful. Save that for a follow up effort in a new PR? I'd imagine that's introducing a new cli flag (and env var) to control that?
Co-authored-by: Brad Davidson <[email protected]> Signed-off-by: Dan P. <[email protected]>
Co-authored-by: Brad Davidson <[email protected]> Signed-off-by: Dan P. <[email protected]>
Signed-off-by: Dan Pock <[email protected]>
pkg/controllers/controllers.go
Outdated
| klog.Info("Starting helm controller with no namespace") | ||
| } else { | ||
| klog.Infof("Starting helm controller in namespace %s", systemNamespace) | ||
| klog.Infof("Starting %s in namespace %s", controllerName, systemNamespace) |
There was a problem hiding this comment.
Maybe we should clarify these... for namespace %s and for all namespaces with lock in %s or something like that?
There was a problem hiding this comment.
Oh I like that idea a lot. Nice.
There was a problem hiding this comment.
Updated them now - until we add the feature to control lock namespace that's just a string. Once we add it we can swap that string for the variable lockNamespace (or w/e is implemented).
mallardduck
force-pushed
the
adjust-lock-for-controller-name
branch
from
2807358 to
7e93b23
Compare
Signed-off-by: Dan Pock <[email protected]>
mallardduck
force-pushed
the
adjust-lock-for-controller-name
branch
from
7e93b23 to
7917893
Compare
I don't have access to merge here currently, I'm happy to join the effort and do the needful to gain access. Just not sure what that looks like, if can let me know? (feel free to start that thread in slack if that make sense) Either way, I'm happy this PR is good to go for whenever someone with merge access is ready to launch. 🥂 |
Follow up PR to: #258
This is a more simple change that's still helpful after my confusion around the lease mechanism was clarified.