Add generic Ref, Deref and Equal to the pointer package #269

alculquicondor · 2022-11-02T21:20:32Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add Ref, Deref and Equal that use generics. This simplifies implementation and also allow to have pointers of enums or arbitrary types.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

Add Ref, Deref and Equal to pointer package to handle pointers generically

alculquicondor · 2022-11-03T13:12:14Z

Ahhh, actually, this seems to be against the policy https://github.com/kubernetes/community/blob/master/sig-architecture/generics.md#generics-policy

alculquicondor · 2022-11-03T13:15:22Z

/hold
until 2023-07-28, as per https://kubernetes.io/releases/patch-releases/#1-24

/lifecycle frozen

k8s-ci-robot · 2022-11-03T13:15:24Z

@alculquicondor: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/hold
until 2023-07-28, as per https://kubernetes.io/releases/patch-releases/#1-24

/lifecycle frozen

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.

thockin · 2022-11-08T00:49:35Z

Are we doing generics in utils now?

@logicalhan @dims @BenTheElder

BenTheElder · 2022-11-08T00:54:09Z

No, we are not yet, because we still support Kubernetes versions that do not use generics yet. The tentative plan is to permit generics when 1.24 is out of support (or earlier, but only in k/k / staging, not in shared libraries that cross release boundaries).

https://github.com/kubernetes/community/blob/master/sig-architecture/generics.md

BenTheElder · 2022-11-08T00:54:47Z

I think the comments above already note that and are holding the PR until that time though?

thockin · 2022-11-08T01:22:19Z

I missed that. Mea culpa.

alculquicondor · 2022-11-10T18:38:29Z

The guidance is now adjusted to the EoL for 1.23, which is on 2023-02-28

thockin · 2022-11-11T20:59:04Z

pointer/pointer.go

-func Int(i int) *int {
-	return &i
+// Ref returns a pointer to the value.
+func Ref[T any](v T) *T {


Why not "New" ptr.New[int64](12345) is as close as we can get to &int64(12345) or new(int64, 12345)

I liked how complementary Ref and Deref sounded. But New would work too.

How about To? pointer.To(12345) seems even nicer to me...

pointer.New(), pointer.To(), pointer.P() would all be fine to me. We could even rename the pkg to ptr.

can we? I guess we would need to duplicate the package first.

But we are still waiting for 1.24 to go EoL next month.

It wouldn’t have to be duplicated — you could add a generic ptr package, and change pointer to be full of vars pointing to ptr functions.

One benefit of introducing a new generic ptr package instead of updating the existing pointer package would be to simplify migration tracking: any references to utils/pointer would point (hah) to code that should be updated.

I agree.

@skitt would you like to take over? The time for adding these generics is coming next month, and I honestly have limited time to do so.

I can simply close this PR and you can start over with the ptr package approach.

I can take care of this, yes!

Awesome

/close

alculquicondor · 2023-01-13T15:20:13Z

FWIIW, 1.23 is using golang 1.19 now, but I don't think there was a release with it, yet.

alculquicondor · 2023-01-13T15:23:34Z

For sure there has been a release with 1.18 kubernetes/kubernetes#113983

@BenTheElder, can we lift the restriction?

alculquicondor · 2023-01-13T15:24:22Z

cc @liggitt

liggitt · 2023-01-13T15:25:57Z

For sure there has been a release with 1.18 kubernetes/kubernetes#113983

Actually, there hasn't... 1.23 jumped from go1.17 to go1.19 in that PR and doesn't yet have a patch release on that (will be in January's releases).

I still think we should still wait until 1.23/1.24 are out of support since library consumers can still be on the original go minor versions those were released with

alculquicondor · 2023-01-13T15:46:01Z

since library consumers can still be on the original go minor versions those were released with

But if they upgrade the libraries they would need to upgrade golang, or not necessarily?

liggitt · 2023-01-13T15:47:44Z

But if they upgrade the libraries they would need to upgrade golang, or not necessarily?

No, building / testing 1.23 client-go examples with go1.17 and 1.24 with go1.18 still works.

thockin · 2023-01-17T19:34:58Z

So we're NOT accelerating this? Just confirming.

liggitt · 2023-01-18T13:43:07Z

So we're NOT accelerating this? Just confirming.

That would be my vote. I think we should stick to the original timeline, so we don't put ourselves in a position where a bump of k8s.io/utils in the 1.23 or 1.24 release branch would force a library consumer to update go versions on a patch bump.

alculquicondor · 2023-01-18T15:36:44Z

But 1.24 was originally released with go=1.18, which supports generics. So we should good to start using generics when 1.23 goes EoL.

k8s-triage-robot · 2023-04-18T16:28:57Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

shaneutt · 2023-04-19T07:53:41Z

Still holding until July, to my understanding?

/remove-lifecycle stale

skitt · 2023-06-14T15:16:46Z

pointer/pointer.go

-}
+	// IntPtr is a function variable referring to Int.
+	//
+	// Deprecated: Use Int instead.


This might as well point to Ref (or New or To) directly since that’s what people updating deprecated code should end up using.

skitt · 2023-06-14T15:17:05Z

pointer/pointer.go

-}
+	// IntPtrDerefOr is a function variable referring to IntDeref.
+	//
+	// Deprecated: Use IntDeref instead.


Same here: this might as well point to Deref directly. (Same goes for all the other instances.)

k8s-ci-robot · 2023-06-14T15:17:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alculquicondor
Once this PR has been reviewed and has the lgtm label, please ask for approval from thockin. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pointer/OWNERS

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

k8s-ci-robot · 2023-06-15T16:00:22Z

@alculquicondor: Closed this PR.

In response to this:

Awesome

/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.

This simplifies the implementation and adds support for pointers to enums and arbitrary types. To allow a complete migration from pointer to ptr, this also moves AllPtrFieldsNil. The ptr.AllPtrFieldsNil "ptr" repetition is preserved since the function only looks at pointer fields. Existing deprecation notices are updated to point to the new functions, but the remaining pointer functions aren't deprecated (yet) to avoid having a large amount of imposed changes in downstream projects where linting forbids deprecated functions. Existing test code is preserved as-is to "prove" the correctness of the migration. This was mostly written by Aldo Culquicondor; see kubernetes#269 for context. Signed-off-by: Stephen Kitt <skitt@redhat.com>

This simplifies the implementation and adds support for pointers to enums and arbitrary types. To allow a complete migration from pointer to ptr, this also moves AllPtrFieldsNil. The ptr.AllPtrFieldsNil "ptr" repetition is preserved since the function only looks at pointer fields. Existing deprecation notices are updated to point to the new functions, and the pointer package as a whole is deprecated. Existing test code is preserved as-is to "prove" the correctness of the migration. This was mostly written by Aldo Culquicondor; see kubernetes#269 for context. Signed-off-by: Stephen Kitt <skitt@redhat.com>

Add generic Ref, Deref and Equal

066ff54

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 2, 2022

k8s-ci-robot requested review from apelisse and stewart-yu November 2, 2022 21:20

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 2, 2022

alculquicondor changed the title ~~Add generic Ref, Deref and Equal~~ Add generic Ref, Deref and Equal to the pointer package Nov 2, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2022

thockin self-assigned this Nov 8, 2022

thockin reviewed Nov 11, 2022

View reviewed changes

EdDev mentioned this pull request Dec 19, 2022

pointer: Introduce a generic pointer package kubevirt/kubevirt#8936

Merged

shaneutt mentioned this pull request Mar 16, 2023

Add a conformance test that checks adding and removing listeners kubernetes-sigs/gateway-api#1827

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2023

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 19, 2023

skitt mentioned this pull request Apr 26, 2023

Cluster lifecycle: use new intstr functions kubernetes/kubernetes#117630

Merged

alculquicondor mentioned this pull request Apr 26, 2023

intstr: add constructor from int32, deprecate int constructor kubernetes/kubernetes#116665

Merged

skitt suggested changes Jun 14, 2023

View reviewed changes

k8s-ci-robot closed this Jun 15, 2023

skitt mentioned this pull request Jun 15, 2023

Add generic ptr.To, ptr.Deref, ptr.Equal #283

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic Ref, Deref and Equal to the pointer package #269

Add generic Ref, Deref and Equal to the pointer package #269

alculquicondor commented Nov 2, 2022

alculquicondor commented Nov 3, 2022

alculquicondor commented Nov 3, 2022

k8s-ci-robot commented Nov 3, 2022

thockin commented Nov 8, 2022

BenTheElder commented Nov 8, 2022

BenTheElder commented Nov 8, 2022

thockin commented Nov 8, 2022

alculquicondor commented Nov 10, 2022

thockin Nov 11, 2022

alculquicondor Nov 11, 2022

skitt Jun 14, 2023

thockin Jun 14, 2023

alculquicondor Jun 14, 2023

skitt Jun 15, 2023

skitt Jun 15, 2023

alculquicondor Jun 15, 2023

skitt Jun 15, 2023

alculquicondor Jun 15, 2023

alculquicondor commented Jan 13, 2023 •

edited

Loading

alculquicondor commented Jan 13, 2023

alculquicondor commented Jan 13, 2023

liggitt commented Jan 13, 2023 •

edited

Loading

alculquicondor commented Jan 13, 2023

liggitt commented Jan 13, 2023 •

edited

Loading

thockin commented Jan 17, 2023

liggitt commented Jan 18, 2023

alculquicondor commented Jan 18, 2023

k8s-triage-robot commented Apr 18, 2023

shaneutt commented Apr 19, 2023

skitt Jun 14, 2023 •

edited

Loading

skitt Jun 14, 2023

k8s-ci-robot commented Jun 14, 2023

k8s-ci-robot commented Jun 15, 2023

Add generic Ref, Deref and Equal to the pointer package #269

Add generic Ref, Deref and Equal to the pointer package #269

Conversation

alculquicondor commented Nov 2, 2022

alculquicondor commented Nov 3, 2022

alculquicondor commented Nov 3, 2022

k8s-ci-robot commented Nov 3, 2022

thockin commented Nov 8, 2022

BenTheElder commented Nov 8, 2022

BenTheElder commented Nov 8, 2022

thockin commented Nov 8, 2022

alculquicondor commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Jan 13, 2023 • edited Loading

alculquicondor commented Jan 13, 2023

alculquicondor commented Jan 13, 2023

liggitt commented Jan 13, 2023 • edited Loading

alculquicondor commented Jan 13, 2023

liggitt commented Jan 13, 2023 • edited Loading

thockin commented Jan 17, 2023

liggitt commented Jan 18, 2023

alculquicondor commented Jan 18, 2023

k8s-triage-robot commented Apr 18, 2023

shaneutt commented Apr 19, 2023

skitt Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 14, 2023

k8s-ci-robot commented Jun 15, 2023

alculquicondor commented Jan 13, 2023 •

edited

Loading

liggitt commented Jan 13, 2023 •

edited

Loading

liggitt commented Jan 13, 2023 •

edited

Loading

skitt Jun 14, 2023 •

edited

Loading