⚠ Replace clusterName string parameter with the explicit type multicluster.ClusterName

[ntnn]

Typing an identifier has many benefits. It avoids passing the wrong variable at the wrong place, it aids in readability (e.g. we have many places where a cluster name is just referred to as key - finding out that it is a cluster name then needs some digging, especially when the helper functions that generated the data being iterated over is also very named very nondescript), etcpp

[embik]

I feel like we had this before, but that was maybe with a generic. @sttts any objections to this?

[sttts]

A string wrapper type I can see. I was always against making the cluster name generic, i.e. with a type parameter. I tried that once on a branch and it was awful in usability. But this type ClusterName string is fine.

[ntnn]

I feel like we had this before, but that was maybe with a generic. @sttts any objections to this?

Seems like it: #2

Personally I'm more a fan of strong typing identifiers and using generics to support similar types rather than different identifiers.

[embik]

Okay, sounds good. Since the PR title is the release note in this project, can we maybe make it more descriptive? Something that says "Replace clusterName string parameter for providers with multicluster.ClusterName"?

[embik]

/approve

off we go 🚀

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: aea305e1a3767c7fd9899c2d984c257d744c8277

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: embik, ntnn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [embik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment