⚠️ Rename leader election config fields #2637
Conversation
Change the names of the leader election config fields to be more explanatory and reflective of what they actually do: - `LeaderElectionNamespace` -> `LeaderElectionLockNamespace` since it actually is the namespace for the lock. - `LeaderElectionID` -> `LeaderElectionLockName` since it actually is the resource name of the lock. - Also updated `pkg/leaderelection` to remove stuttering in the field names since the type is already `leaderelection.Options` and no need to repeat the `LeaderElection` prefix.
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ahmetb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
|
@ahmetb: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Failing api-diff, unsure how to deal with removals and utilize this tool to not flag what is being renamed. @joelanford 👀 |
| // LeaderElectionNamespace determines the namespace in which the leader | ||
| // election resource will be created. | ||
| LeaderElectionNamespace string | ||
| // LeaderElectionLockNamespace determines the namespace of the leader election lock |
There was a problem hiding this comment.
While I do agree that the new names are better, renaming these will result in a lot of churn for consumers, I am not sure if that is worth it
There was a problem hiding this comment.
Yeah totally valid. It doesn't offer any net new improvement.
There was a problem hiding this comment.
Agreed with @alvaroaleman.
If we think the name change really is worth it, we could introduce the new fields, deprecate the old ones, and then go a few minor releases before deleting the old ones. Still churn, but gives folks warning/time. That would bring its own complexity in the interim though (multiple fields that do the same thing, coding for mutual exclusivity, etc.)
There was a problem hiding this comment.
Would leaving a comment in the docs for this be better? I don't see this getting merged being that the churn associated with this and the complexity @joelanford mentioned with trying to promote these fields and deprecating others would help either.
There was a problem hiding this comment.
To echo other's comments, given that the field are self explanatory, maybe not 100% accurate I guess (for historical reasons these have changed the internal meaning); could we add more information on the comment, rather than renaming them?
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
Yes, this is just an informational test that helps us ensure we avoid accidental API breakage. A failure for intentional API breakage does not block merge. |
|
Closing based on the decision on the proposal. |
|
@ahmetb: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changing the
manager.Optionsandleaderelection.Optionsfield names around leader election configuration to be more explanatory and reflective of what they actually do.LeaderElectionNamespace->LeaderElectionLockNamespacesince it actually is the namespace for the lock.LeaderElectionID->LeaderElectionLockNamesince it actually is the resource name of the lock.Also updated
pkg/leaderelectionto remove stuttering in the field names since the type is alreadyleaderelection.Optionsand there is no need to repeat theLeaderElectionprefix.Closes #2636.