Introduce plugins.TypedName to be used for Plugin base implementation

[elevran]

Reduce boilerplate in Plugins by embedding a TypedName into each, implementing the Plugin methods.

[k8s-ci-robot]

Hi @elevran. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[netlify]

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	3766b51
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/68652446b0427f000859b18f
😎 Deploy Preview	https://deploy-preview-1086--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[nirrozenbaum]

/ok-to-test

[ahg-g]

Not suggesting to change anything now, but the explicit Plugin "Type" parameter is duplicating the builtin type name that we can get via the reflect pkg; what if we add a common function in the plugin interface that uses reflect.TypeOf to get the type instead of having the plugins to define a constant?

[elevran]

Not suggesting to change anything now, but the explicit Plugin "Type" parameter is duplicating the builtin type name that we can get via the reflect pkg; what if we add a common function in the plugin interface that uses reflect.TypeOf to get the type instead of having the plugins to define a constant?

Can see Pros and Cons to this (and these depend on implementation):

• using the full package path guarantees uniqueness (whereas strings could collide).
• could potentially save some boilerplate (but would be hard to delegate to an embedded struct and reuse implementation)
• users would need to know the internal Go type (e.g., do we use full package path)?
• types should make sense and be more descriptive (e.g., currently the cache prefix is just called Plugin...)

Overall, would need more careful consideration but I can see some good outcomes.

[nirrozenbaum]

I understand the motivation behind this, but this function looks not natural.

[nirrozenbaum]

I’d argue it’s better to put back a one liner TypedName() function in each plugin..
yes, it adds “boilerplate” code (hard to say this on one line), but more aligned with how things are usually written in go.

[elevran]

I think your point is that a Plugin HAS-A TypedName and not IS-A TypedName (which is what embedding models).
We could add a BasePlugin struct that implements GetTypedName() generically and then embed that into Plugin, but I would prefer to delay that until there's more "behavior" in the base plugin.

[elevran]

@nirrozenbaum hopefully last iteration: made TypedName a private field and not an embedded struct

[nirrozenbaum]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elevran, nirrozenbaum

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:

• OWNERS [nirrozenbaum]

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