add generic sets to utils #243

logicalhan · 2022-04-19T21:27:57Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We have a sets utils in pkg in k8s/k8s. This refactors that implementation to use generics instead of code-generation and moves the library here.

Release note:

NONE

liggitt · 2022-04-20T14:24:00Z

utils now requires golang 1.18

A couple thoughts:

as long as we have supported Kubernetes release branches that use k8s.io/utils and aren't on go1.18, I would not expect us to make the minimum version go1.18... can we use go version build tags or something similar to scope these files to go1.18 while making the rest of the module consumable by earlier versions of go?
we have strict go API compatibility checks on this repo... is this the best place to start experimenting with go APIs using generics? we'll be stuck with any mistakes we're unhappy with

apelisse · 2022-04-22T15:59:58Z

as long as we have supported Kubernetes release branches that use k8s.io/utils and aren't on go1.18, I would not expect us to make the minimum version go1.18... can we use go version build tags or something similar to scope these files to go1.18 while making the rest of the module consumable by earlier versions of go?

Agreed, if we can have some form of conditional compilation and isolate the packages that require go1.18, that'd be nice. Otherwise, we could also branch into a requires-1.18 vs no-1.18. In case we have to make a patch to an older version, we could apply it to the no-1.18 and merge that change back into the 1.18 branch, the process is a little more involved. We can also put that process in place lazily when/if we eventually need it. How often have we needed to patch utils in a minor release?

we have strict go API compatibility checks on this repo... is this the best place to start experimenting with go APIs using generics? we'll be stuck with any mistakes we're unhappy with

The thing is that yeah, utils are usually the best place to start using generics :-(

k8s-triage-robot · 2022-07-21T16:59:04Z

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

This bot triages issues and 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 issue is closed

You can:

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

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

/lifecycle stale

k8s-triage-robot · 2022-08-20T17:55:03Z

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

This bot triages issues and 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 issue is closed

You can:

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

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

/lifecycle rotten

k8s-triage-robot · 2022-09-19T18:14:31Z

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

This bot triages issues and 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 issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot · 2022-09-19T18:14:36Z

@k8s-triage-robot: Closed this PR.

In response to this:

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

This bot triages issues and 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 issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

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

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

We let this age-out. When do we bring it back?

logicalhan · 2022-11-08T16:57:26Z

/reopen

We let this age-out. When do we bring it back?

1.27 (so next release)

k8s-ci-robot · 2022-11-08T16:57:32Z

@logicalhan: Reopened this PR.

In response to this:

/reopen

We let this age-out. When do we bring it back?

1.27 (so next release)

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.

logicalhan · 2022-11-08T16:58:03Z

/lifecycle frozen

k8s-ci-robot · 2022-11-08T16:58:04Z

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

In response to this:

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

logicalhan · 2022-11-08T16:58:18Z

/remove-lifecycle rotten

logicalhan · 2022-11-08T19:29:26Z

/close

k8s-ci-robot · 2022-11-08T19:29:36Z

@logicalhan: Closed this PR.

In response to this:

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

skitt · 2023-04-27T13:59:15Z

sets/set.go

+
+import "sort"
+
+type Ordered interface {


The apimachinery implementation uses comparable instead (which requires Go 1.20 to work properly).

sets/set.go

thockin · 2023-04-27T18:00:15Z

The doc on comparable is ... tricky to digest.

skitt · 2023-04-27T19:06:38Z

The doc on comparable is ... tricky to digest.

That’s putting it mildly! But I think it’s the right approach in this case (assuming a Go 1.20 bump is acceptable).

thockin · 2023-04-27T19:51:47Z

I'm trying to digest why it's OK to runtime-panic on any and how to apply the dirty trick at the end of the doc

thockin · 2023-04-27T20:08:18Z

Like: https://go.dev/play/p/vBEo5hg2mnu

It seems BETTER in 1.19 than 1.20

thockin · 2023-04-27T20:19:34Z

sets/set.go

+limitations under the License.
+*/
+
+package sets


Since we're nitpicking, why not set - set.Set[int] or even set.S[int] ?

Or should this be containers.Set[int] or something? stl.Set[int] ? :)

I renamed the package to set.

logicalhan · 2023-05-05T19:31:48Z

So basically my plan is twofold:

(1) introduce sets using ordered type so that we can have set.SortedList
(2) once we can bump utils to golang 1.20, then I will introduce a set[comparable] which has the exact API as set[ordered] without the SortedList method. Then I will create an API compatible version of set[ordered] which delegates all calls underneath to set[comparable] and then implement SortedList on set[ordered].

thockin

nits

sets/set.go

set/ordered.go

set/set.go

set/set_test.go

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-05-05T20:15:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logicalhan, 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:

~~OWNERS~~ [thockin]

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

pohly · 2023-05-15T07:05:36Z

once we can bump utils to golang 1.20, then I will introduce a set[comparable] which has the exact API as set[ordered] without the SortedList method. Then I will create an API compatible version of set[ordered] which delegates all calls underneath to set[comparable] and then implement SortedList on set[ordered].

Is there an issue open for tracking that? Do you already have a prototype?

Following kubernetes/utils#243 it's likely that the k8s.io/utils version will end up being the canonical one. Signed-off-by: Stephen Kitt <skitt@redhat.com>

k8s-ci-robot requested review from andrewsykim and apelisse April 19, 2022 21:28

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

liggitt mentioned this pull request Jul 28, 2022

Add generics policy for kubernetes kubernetes/community#6693

Merged

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 20, 2022

k8s-ci-robot closed this Sep 19, 2022

thockin self-assigned this Nov 8, 2022

k8s-ci-robot reopened this Nov 8, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 8, 2022

k8s-ci-robot closed this Nov 8, 2022

skitt reviewed Apr 27, 2023

View reviewed changes

thockin reviewed Apr 27, 2023

View reviewed changes

thockin mentioned this pull request May 3, 2023

rethink sets.List() / sets.Set.UnsortedList() naming? kubernetes/kubernetes#117673

Open

make this implementation of sets compatible with the one in apimachinery

46762cf

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 5, 2023

logicalhan force-pushed the generics branch from 2a8b9a2 to 4754579 Compare May 5, 2023 19:34

fix lint issue

c3703b2

logicalhan force-pushed the generics branch from 4754579 to c3703b2 Compare May 5, 2023 19:37

thockin reviewed May 5, 2023

View reviewed changes

address tim's comments

10cedcc

thockin reviewed May 5, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2023

k8s-ci-robot merged commit 9f67429 into kubernetes:master May 5, 2023
10 checks passed

liggitt mentioned this pull request May 10, 2023

add generic methods for integer package #280

Closed

This was referenced Jun 21, 2023

x/exp/apidiff: incorrect behaviour with user defined type constraints golang/go#60911

Closed

Broken CI job: apidiff #284

Closed

Revert "add generic sets to utils" #285

Closed

skitt mentioned this pull request Jul 4, 2023

Use k8s.io/utils set instead of apimachinery sets operator-framework/operator-sdk#6486

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add generic sets to utils #243

add generic sets to utils #243

logicalhan commented Apr 19, 2022 •

edited

liggitt commented Apr 20, 2022

apelisse commented Apr 22, 2022

k8s-triage-robot commented Jul 21, 2022

k8s-triage-robot commented Aug 20, 2022

k8s-triage-robot commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

thockin commented Nov 8, 2022

logicalhan commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

logicalhan commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

logicalhan commented Nov 8, 2022

logicalhan commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

skitt Apr 27, 2023

thockin commented Apr 27, 2023

skitt commented Apr 27, 2023

thockin commented Apr 27, 2023

thockin commented Apr 27, 2023

thockin Apr 27, 2023

logicalhan May 5, 2023

logicalhan commented May 5, 2023

thockin left a comment

thockin left a comment

k8s-ci-robot commented May 5, 2023

pohly commented May 15, 2023

add generic sets to utils #243

add generic sets to utils #243

Conversation

logicalhan commented Apr 19, 2022 • edited

liggitt commented Apr 20, 2022

apelisse commented Apr 22, 2022

k8s-triage-robot commented Jul 21, 2022

k8s-triage-robot commented Aug 20, 2022

k8s-triage-robot commented Sep 19, 2022

k8s-ci-robot commented Sep 19, 2022

thockin commented Nov 8, 2022

logicalhan commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

logicalhan commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

logicalhan commented Nov 8, 2022

logicalhan commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

skitt Apr 27, 2023

Choose a reason for hiding this comment

thockin commented Apr 27, 2023

skitt commented Apr 27, 2023

thockin commented Apr 27, 2023

thockin commented Apr 27, 2023

thockin Apr 27, 2023

Choose a reason for hiding this comment

logicalhan May 5, 2023

Choose a reason for hiding this comment

logicalhan commented May 5, 2023

thockin left a comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 5, 2023

pohly commented May 15, 2023

logicalhan commented Apr 19, 2022 •

edited