cp pointers util from kubernetes/kubernetes #41

stewart-yu · 2018-07-18T01:29:27Z

We need move the k8s.io/kubernetes/pkg/util/pointer package to k8s.io/utils/ repo, more details please refer to PR in kubernetes repo.

Following the instruction, this PR is the first step

stewart-yu · 2018-07-18T01:33:49Z

seems no OWNER file in this repo
/assign @thockin @luxas @sttts
PTAL, related kubernetes repo PR, thanks

sttts · 2018-07-18T08:33:59Z

README.md

@@ -1,5 +1,6 @@
 # Utils

+


nit: why this empty line?

sttts · 2018-07-18T08:34:32Z

pointer/pointer.go

+// BoolPtr returns a pointer to a bool
+func BoolPtr(b bool) *bool {
+	return &b
+}


StringPtr. Don't we have that 1000 times in kube?

sure, add it now

luxas

/lgtm
Thanks!

stewart-yu · 2018-07-25T01:09:29Z

/cc @thockin
for review, thanks

apelisse · 2018-07-25T22:01:30Z

README.md

@@ -45,6 +45,7 @@ an existing package to this repository.

 - [Clock](/clock) provides an interface for time-based operations.  It allows
  mocking time for testing.
+- [Pointers](/pointers) provides some functions for pointer-based operations.


For some reason, we've kept an empty line between each module (don't ask me why), but maybe we should keep it consistent?

apelisse · 2018-07-25T22:08:06Z

pointer/pointer.go

+	return true
+}
+
+// Int32Ptr returns a pointer to an int32


AFAICR, Tim gave this as an example of what we shouldn't have in this repo (might remember it wrong, I'll look it up)

seems Int32Ptr() had been used, if do not put here, which file fo put it?

The problem is not that it's not being used, the problem is that you're trying to share code with very little complexity.

It's gonna be used by many packages, and is generally useful, although very simple.

apelisse · 2018-07-25T22:11:03Z

kubernetes/kubernetes#48209 (comment)

luxas · 2018-07-26T17:43:22Z

On the other hand @apelisse kubernetes/kubernetes#66010 (review) :)

This seems fine but why apimachinery and not https://github.com/kubernetes/utils ? I might clean up some of the APIs, if you want to put a little polish into it...

apelisse · 2018-07-26T17:44:59Z

I guess I can't argue with that :-)

k8s-ci-robot · 2018-07-26T17:45:43Z

New changes are detected. LGTM label has been removed.

apelisse · 2018-07-26T17:47:17Z

pointer/pointer_test.go

+		{(*struct{})(nil), true},
+	}
+	for i, tc := range testCases {
+		if AllPtrFieldsNil(tc.obj) != tc.expected {


This could use t.Run

luxas · 2018-07-26T17:57:01Z

Can you merge then to unblock us @apelisse :)?

luxas · 2018-07-26T18:02:37Z

Thanks 🎉!

stewart-yu · 2018-07-27T01:01:55Z

Thanks all

sttts · 2018-07-27T08:09:35Z

Finally we have a generic pointer package 🎉🚀

sttts · 2018-07-27T08:10:13Z

pointer/pointer.go

+}
+
+// StringPtr returns a pointer to the passed string.
+func StringPtr(s string) *string {


float64 is missing 😱

sorry, little knowledge about it, make be as follow-up
In generic, there are StringSlicePtr StringMapPtr Float32Ptr Float64Ptr , does we need some of them or all to make pointer packages contained all pointer-oprater?

thockin · 2018-07-27T15:39:20Z

This is in so I won't be a pain about it, but this is the sort of dependency that causes very interconnected dep graphs. Any tool that uses this now has to vendor this whole repo. Part of the go ethos is that it's better to copy a little code than to take a dependency. Let's please not let this accumulate tons of random crap.

luxas · 2018-07-27T15:53:32Z

No agree we shouldn't put "random crap" in this repo, but this exact utilpointer code is referenced by most of our API packages that deal with defaulting, so this is kinda-needed unless we want to start doing stuff like this in our defaulting packages (that will now live in n different repos)

trueVar := true
obj.SomeBoolPointerField = &trueVar

vs

obj.SomeBoolPointerField = utilpointer.BoolVar(true)

Well you know this already, but anyway I've made the argument for this now.

For me this could have moved to k8s.io/apimachinery equally fine but you said in kubernetes/kubernetes#66010 (review)

This seems fine but why apimachinery and not https://github.com/kubernetes/utils ? I might clean up some of the APIs, if you want to put a little polish into it...

So I'm a bit confused.
In any case I'm of the opinion this needs to live somewhere to not be a pain for the defaulting code for the ComponentConfigs.

Any tool that uses this now has to vendor this whole repo

hmm I had the idea one can vendor individual packages, not the whole repo? @sttts had also done some experimenting with this I think wrt the ComponentConfig KEP in general

thockin · 2018-07-27T15:58:08Z

Lucas, you should know by now that I never claim to be 100% self-consistent :) You can vendor just one package, but you have to download and examine the whole repo. Some tools used to not do subdir-vendoring, but I think we're past that.

…

On Fri, Jul 27, 2018 at 8:53 AM Lucas Käldström ***@***.***> wrote: No agree we shouldn't put "random crap" in this repo, but this exact utilpointer code is referenced by most of our API packages that deal with defaulting, so this is kinda-needed unless we want to start doing stuff like this in our defaulting packages (that will now live in n different repos) trueVar := true obj.SomeBoolPointerField = &trueVar vs obj.SomeBoolPointerField = utilpointer.BoolVar(true) Well you know this already, but anyway I've made the argument for this now. For me this could have moved to k8s.io/apimachinery equally fine but you said in kubernetes/kubernetes#66010 (review) <kubernetes/kubernetes#66010 (review)> This seems fine but why apimachinery and not https://github.com/kubernetes/utils ? I might clean up some of the APIs, if you want to put a little polish into it... So I'm a bit confused. In any case I'm of the opinion this needs to live somewhere to not be a pain for the defaulting code for the ComponentConfigs. Any tool that uses this now has to vendor this whole repo hmm I had the idea one can vendor individual packages, not the whole repo? @sttts <https://github.com/sttts> had also done some experimenting with this I think wrt the ComponentConfig KEP in general — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVOpeep3-yckjPFLBdljOdLM-8PvXks5uKzd8gaJpZM4VTzLK> .

luxas · 2018-07-27T16:07:01Z

Lucas, you should know by now that I never claim to be 100% self-consistent

Hahah, yeah ;). Can't say I'm always super consistent with reviews either lol

You can vendor just one package, but you have to download and examine the
whole repo. Some tools used to not do subdir-vendoring, but I think we're
past that.

I don't think this will be a problem as anything should be able to depend on k8s.io/utils, this is used by multiple k8s consumers in various repos, the package has a well-defined scope, and all modern vendoring systems I know of supports pulling individual packages.

@thockin If this sounds good to you, go ahead and approve kubernetes/kubernetes#66284 which follows this up in the k/k repo ;)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2018

k8s-ci-robot assigned luxas, sttts and thockin Jul 18, 2018

stewart-yu mentioned this pull request Jul 18, 2018

Move the k8s.io/kubernetes/pkg/util/pointer package to k8s.io/utils/pointer kubernetes/kubernetes#66284

Merged

sttts reviewed Jul 18, 2018

View reviewed changes

README.md Outdated

@@ -1,5 +1,6 @@

# Utils

Copy link

Contributor

sttts Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why this empty line?

sttts reviewed Jul 18, 2018

View reviewed changes

cp pointers util from kubernetes/kubernetes

3289602

stewart-yu force-pushed the stewart-utils-pointers branch from 3f07872 to 3289602 Compare July 18, 2018 08:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2018

luxas approved these changes Jul 18, 2018

View reviewed changes

k8s-ci-robot requested a review from thockin July 25, 2018 01:09

apelisse reviewed Jul 25, 2018

View reviewed changes

README: Add extra-space to keep list consistent

53291e9

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2018

apelisse reviewed Jul 26, 2018

View reviewed changes

apelisse approved these changes Jul 26, 2018

View reviewed changes

apelisse merged commit 66066c8 into kubernetes:master Jul 26, 2018

sttts reviewed Jul 27, 2018

View reviewed changes

stewart-yu mentioned this pull request Jul 27, 2018

add FloatPtr to pointer packages #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cp pointers util from kubernetes/kubernetes #41

cp pointers util from kubernetes/kubernetes #41

stewart-yu commented Jul 18, 2018

stewart-yu commented Jul 18, 2018

sttts Jul 18, 2018

sttts Jul 18, 2018

stewart-yu Jul 18, 2018

luxas left a comment

stewart-yu commented Jul 25, 2018

apelisse Jul 25, 2018

apelisse Jul 25, 2018

stewart-yu Jul 26, 2018

apelisse Jul 26, 2018

luxas Jul 26, 2018

apelisse commented Jul 25, 2018

luxas commented Jul 26, 2018

apelisse commented Jul 26, 2018

k8s-ci-robot commented Jul 26, 2018

apelisse Jul 26, 2018

luxas commented Jul 26, 2018

luxas commented Jul 26, 2018

stewart-yu commented Jul 27, 2018

sttts commented Jul 27, 2018

sttts Jul 27, 2018

stewart-yu Jul 27, 2018

thockin commented Jul 27, 2018

luxas commented Jul 27, 2018

thockin commented Jul 27, 2018 via email

luxas commented Jul 27, 2018

cp pointers util from kubernetes/kubernetes #41

cp pointers util from kubernetes/kubernetes #41

Conversation

stewart-yu commented Jul 18, 2018

stewart-yu commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

stewart-yu commented Jul 25, 2018

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

apelisse commented Jul 25, 2018

luxas commented Jul 26, 2018

apelisse commented Jul 26, 2018

k8s-ci-robot commented Jul 26, 2018

Choose a reason for hiding this comment

luxas commented Jul 26, 2018

luxas commented Jul 26, 2018

stewart-yu commented Jul 27, 2018

sttts commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Jul 27, 2018

luxas commented Jul 27, 2018

thockin commented Jul 27, 2018 via email

luxas commented Jul 27, 2018