-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ default a LeaderElectionID #1394
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mengqiy 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 |
This PR is a prerequisite for #1391. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1
didn't support leader election, did it?
Minor comment
LeaderElection: enableLeaderElection, | ||
// TODO(user): You MUST ensure LeaderElectionID is unique for your controller. Otherwise, 2 controllers using | ||
// the same default LeaderElectionID in the same namespace will race and one of them will stop working silently. | ||
LeaderElectionID: "replace-this-with-a-unique-id", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would create a unique string from domain
and repo
, adding some hashing too. Something like "{{ .Domain }}/{{ hash .Repo }}"
. WDYT?
hash
could be something as simple as this example:
import (
"fmt"
"hash/fnv"
)
var hasher = fnv.New32a()
func hash(s string) string {
hasher.Write([]byte(s))
defer hasher.Reset()
return fmt.Sprintf("%x", hasher.Sum(nil))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I need to change file.Template
a bit to make it more flexible to pass the additional FuncMap
around.
type Template interface {
Builder
// GetBody returns the template body
GetBody() string
// SetTemplateDefaults sets the default values for templates
SetTemplateDefaults() error
// GetFuncMap allow each template to provide its own FuncMap.
GetFuncMap() template.FuncMap
}
EDIT: Not sure if this way is the best. Still trying to find a clean way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I would go for:
type CustomTemplate interface {
Template
GetFuncMap() template.FuncMap
}
This way only those that need it have to implement it.
But we could also implement it by having a HashedRepo
field, set it in the SetTemplateDefaults
, and then use it like "{{ .Domain }}/{{ .HashedRepo }}"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Adirio Any thoughts on current implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only NIT would be adding the Template
to the UseCustomFuncMap
@Adirio v1 doesn't support leader election. |
66af610
to
acc145d
Compare
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/assign @estroz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
kubernetes-sigs/controller-runtime#446 removed the defaulting for LeaderElectionID.
It makes sense for controller-runtime. But kubebuilder should still be able to scaffold a project that work out-of-box.
So we will give users some instructions to update it in code comment.