Test: use new intstr functions

[skitt]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

#116665 introduces intstr contructors from int32; this PR updates code to use them.

/hold pending #116665

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @skitt. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

[bart0sh]

/triage accepted
/priority important-longterm

[thockin]

Can you remind me why we didn't just change the original FromInt() to int32? It's not a public API.

[skitt]

Can you remind me why we didn't just change the original FromInt() to int32? It's not a public API.

I’m not sure it would have been tractable for me to convert everything in one PR (just from a review perspective; although at least in this PR most of the changes would have become unnecessary). It also turned out to be useful to keep the int version for a bunch of use-cases involving port numbers — getting that to be consistent is another battle...

[thockin]

It also turned out to be useful to keep the int version for a bunch of use-cases involving port numbers

I think it would be cleaner to call intstr.FromInt(int32(port)) that to have FromInt32() forever.

What about generics?

something like:

https://go.dev/play/p/noMl3HAd1h_z

Since it is internal, this seems legit to me.

[skitt]

Ah, digging into this some more I remember one of the concerns I had — the overflow check in https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go#L59 means that a plain FromInt() from a smaller type, used as FromInt(int32(foo)) for foo int would result in a change in behaviour (albeit extremely unlikely to cause problems in practice).

I like the generic approach to handle all smaller types, and I agree FromInt() is nicer to read than FromInt32() (but less explicit obviously).

[thockin]

This is an internal API - we can either change it and disallow anything larger than int32 via the type system or we can keep the (gross) range check, but that's really a vestigial artifact from pre-generic days.

[skitt]

I’ve done this as #119285, without using generics for two reasons — there are no users of FromInt() with types smaller than int32, and using a generic type means that all numeric constants need to be cast to one of the supported types.

[thockin]

using a generic type means that all numeric constants need to be cast to one of the supported types

How often do we need to use literals? If we use generics you need to cast literals and almost nothing else. If we don't use generics we have to cast almost everywhere, including literals. Why is it better to not use generics?

[skitt]

How often do we need to use literals? If we use generics you need to cast literals and almost nothing else. If we don't use generics we have to cast almost everywhere, including literals. Why is it better to not use generics?

105 times in the current code-base. (#119285 leaves 10 casts from non-literals.)

With the suggested generics,

type smallInt interface {
	int8 | int16 | int32
}

most current uses of non-literals need to be cast anyway (from int). We could use constraints.Integer or constraints.Signed to accept any integer type, relying on the existing comment that it’s up to callers to ensure that the given value fits in an int32; then no casts would ever be needed. That’s effectively what you’re suggesting in #119285 (comment) ;-).

(The code produced for generics isn’t great, see https://godbolt.org/z/WrK57rcP3, but that’s probably not worth worrying about here compared to the convenience factor.)

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 49c40d31e22bc614db98ba6c3b361a1b65317766

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skitt, thockin

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:

• test/OWNERS [thockin]

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