transport/cache: statically assert that tlsCacheKey is comparable

[enj]

Signed-off-by: Monis Khan mok@microsoft.com

/kind cleanup
/triage accepted
/priority backlog
/assign @liggitt @aojea

NONE

[enj]

Made this a new file with a build constraint in case someone was using an older go version.

[aojea]

should we force the go version too?

[enj]

This felt like a "nicer" approach since this check is meant for the Kube devs to keep this stuff from drifting. I am sure over time we will pick up features from go 1.18 and thus naturally force library consumers to newer Go versions.

[enj]

If you add an interface field to tlsCacheKey, this will no longer compile.

[liggitt]

could this be in a _test file and be equally effective?

also, is there a way to construct this that doesn't make a function that infinitely recursively calls itself, like

func isComparable[T comparable]() bool { return true }

var _ = isComparable[tlsCacheKey]()

[enj]

could this be in a _test file and be equally effective?

We don't generally put interface assertions in test files because you only get a failure if you actually compile/run the test. Having it in the regular file makes it impossible to miss.

also, is there a way to construct this that doesn't make a function that infinitely recursively calls itself

Yeah, that is basically what I had originally and I got too clever with my desire to fit in in one line :) (also our CI linter doesn't like that the function is unused so I have to fix it anyway)

[liggitt]

We don't generally put interface assertions in test files

git grep "var _ = " | grep _test.go disagrees :)

because you only get a failure if you actually compile/run the test.

Which we do, right?

[enj]

We don't generally put interface assertions in test files

git grep "var _ = " | grep _test.go disagrees :)

Hence I used generally. When we really want to make sure stuff does not drift we do it in the regular code like:

kubernetes/pkg/registry/core/service/storage/storage.go

Lines 70 to 76 in 664f0f5

	var (
	_ rest.CategoriesProvider = &REST{}
	_ rest.ShortNamesProvider = &REST{}
	_ rest.StorageVersionProvider = &REST{}
	_ rest.ResetFieldsStrategy = &REST{}
	_ rest.Redirector = &REST{}
	)

because you only get a failure if you actually compile/run the test.

Which we do, right?

Why push an assertion down to a later time?

[enj]

/retest

[aojea]

/lgtm

TIL you can force this check at build time

[liggitt]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, liggitt

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:

• staging/src/k8s.io/client-go/OWNERS [liggitt]

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

[liggitt]

I can't tell if the go1.20 changes make it possible for this to fail at runtime:

from https://tip.golang.org/doc/go1.20#language

Comparable types (such as ordinary interfaces) may now satisfy comparable constraints, even if the type arguments are not strictly comparable (comparison may panic at runtime). This makes it possible to instantiate a type parameter constrained by comparable (e.g., a type parameter for a user-defined generic map key) with a non-strictly comparable type argument such as an interface type, or a composite type containing an interface type.

[enj]

I can't tell if the go1.20 changes make it possible for this to fail at runtime:

from https://tip.golang.org/doc/go1.20#language

Comparable types (such as ordinary interfaces) may now satisfy comparable constraints, even if the type arguments are not strictly comparable (comparison may panic at runtime). This makes it possible to instantiate a type parameter constrained by comparable (e.g., a type parameter for a user-defined generic map key) with a non-strictly comparable type argument such as an interface type, or a composite type containing an interface type.

We need to tweak our code to keep the static check: golang/go#56548 (comment)

[liggitt]

We need to tweak our code to keep the static check: golang/go#56548 (comment)

I assume that's something we can do now in master ahead of go1.20?

[enj]

We need to tweak our code to keep the static check: golang/go#56548 (comment)

I assume that's something we can do now in master ahead of go1.20?

Yeah I just had not gotten around to it yet.