Proposal: Naming consistency for leader election lock options

[ahmetb]

This is probably a very minor naming suggestion but when I first started out using controller-runtime, the LeaderElectionNamespace option's identifier confused me a bit as to what it does and made me look under the covers to see how it is used.

I think clarifying this as follows would address the problem:

@@ -176,13 +176,13 @@ type Options struct {
 	// act on the same resources concurrently.
 	LeaderElectionResourceLock string
 
-	// LeaderElectionNamespace determines the namespace in which the leader
+	// LeaderElectionLockNamespace determines the namespace in which the leader
-	// election resource will be created.
+	// election lock resource will be created.
-	LeaderElectionNamespace string
+	LeaderElectionLockNamespace string
 
-	// LeaderElectionID determines the name of the resource that leader election
+	// LeaderElectionLockID determines the name of the resource that leader election
 	// will use for holding the leader lock.
-	LeaderElectionID string
+	LeaderElectionLockID string
 
 	// LeaderElectionConfig can be specified to override the default configuration
 	// that is used to build the leader election client.

If we go through with this, I think changing LeaderElectionLockID → LeaderElectionLockName would be quite useful as well (since ID is not the Kubernetes term for unique resource names).

I understand these names have been there since the inception, but since I observed this repo regularly changes Options structs (and this is particularly not a troublesome migration), I'm proposing this. Thoughts?

/kind feature

[troy0820]

Would we target this for v0.17 or v0.18?

Naming would provide clarity if that's the intent and not change any functionality. I agree that being concise may help future users around the unclear options are defined.

/kind feature

[ahmetb]

This is a breaking change so we'd probably target the next minor version?
Since the direction sounds good, following up with a patch: #2637.

[troy0820]

Should this be closed because of the comments in the PR?

Are we closing the PR?

@ahmetb @alvaroaleman

[ahmetb]

Yeah let's close this. Benefits are minimal.
/close

[k8s-ci-robot]

@ahmetb: Closing this issue.

In response to this:

Yeah let's close this. Benefits are minimal.
/close

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.