Move pkg util strings/keymutex from kubernetes kubernetes #55

dims · 2018-10-19T11:17:40Z

Move https://github.com/kubernetes/kubernetes/tree/master/pkg/util/strings to k8s.io/utils/strings
Move https://github.com/kubernetes/kubernetes/tree/master/pkg/util/keymutex to k8s.io/utils/keymutex

This is to help with the larger effort around breaking up cloud providers and moving them out of tree.

Fixes #62

dims · 2018-10-19T11:18:36Z

/assign @thockin
cc @mcrute @andrewsykim

k8s-ci-robot · 2018-10-19T11:18:43Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

dims · 2018-10-19T11:22:20Z

related to kubernetes/kubernetes#69585

mcrute

The README says that this package is supposed to be go gettable so it might be worth dropping the Bazel BUILD files. I ended up removing them from the packages I've imported.

mcrute · 2018-10-19T14:47:06Z

strings/strings.go

+}
+
+// isVowel returns true if the rune is a vowel (case insensitive).
+func isVowel(c rune) bool {


It might be worth exporting this since it could be used to eliminate some duplicate code in apiserver

mcrute · 2018-10-19T14:48:16Z

keymutex/hashed.go

+}
+
+// Acquires a lock associated with the specified ID.
+func (km *hashedKeyMutex) LockKey(id string) {


Does golint pass on this package? I thought it complained about comments not starting with function names. New code should be golint clean (he says, realizing now that he forgot to check for his PRs).

travis seems to test at least gofmt and vet. though not go lint. if we want to do golint, we should start by adding it there there and not start on this PR

That's fair. I need to do it with my merged code as well so I'll raise a PR with those changes independent of this one.

Getting clean lint and vet should be a requirement for packages promoted to util

mcrute · 2018-10-19T16:31:57Z

/lgtm

thockin · 2018-10-19T22:54:05Z

keymutex/hashed.go

+}
+
+// Acquires a lock associated with the specified ID.
+func (km *hashedKeyMutex) LockKey(id string) {


Getting clean lint and vet should be a requirement for packages promoted to util

thockin · 2018-10-19T22:59:28Z

strings/escape.go

+	return strings.Replace(in, "~", "/", -1)
+}
+
+// EscapeQualifiedNameForDisk converts a plugin name, which might contain a / into a


Is it just me or are these 2 sets of functions exactly the same? Can we get rid of one? we could also rename this more generally, e.g. EscapePath or EscapeQualifiedName ?

@thockin we enforce vet in travis CI, but we don't do golint. I've filed a PR for that in #56

Done. dropping ForDisk from the names

Dropped ForDisk from method names

thockin · 2018-10-19T23:02:51Z

strings/strings.go

+}
+
+// IsVowel returns true if the rune is a vowel (case insensitive).
+func IsVowel(c rune) bool {


We have a test for this in staging/src/k8s.io/apiserver/pkg/endpoints/installer_test.go and that pkg seems to be the only user of this function. Oh, and they have their own copy of this. Can we nuke this?

Ack. removing this

… operations.

Replace funtions with strings pkg Fix all references and rename the pkg

Refactoring resource container

For example: "a Ingress" > "an Ingress"

anymore. See kubernetes/kubernetes/pull/66442 for discussions.

Signed-off-by: Christian Schlotter <[email protected]>

Change-Id: I33c28cb65b4915b5f037c93bf62fda3f5b4c804a

- Move https://github.com/kubernetes/kubernetes/tree/master/pkg/util/strings to k8s.io/utils/strings - Move https://github.com/kubernetes/kubernetes/tree/master/pkg/util/keymutex to k8s.io/utils/keymutex This is to help with the larger effort around breaking up cloud providers and moving them out of tree. Change-Id: I69ad6a24d05c078fefa8c63df2b9d2c3ab1f5974

dims · 2018-10-26T14:43:44Z

@thockin this is ready. we will have to bypass the CLA stuff though

dims · 2018-11-02T21:21:02Z

/check-cla

thockin · 2018-12-06T17:14:48Z

Why did we close this?

I went looking for it and found it wasn't merged

dims · 2018-12-06T17:19:50Z

re-opened. was not sure what to do about the CLA stuff @thockin

pohly · 2018-12-06T19:54:13Z

Is the problem with the CLA that for some of the authors there's no CLA on record?

Independently today, I proposed keymutex for inclusion into k8s.io/utils (issue #55), so I hope that this can be sorted out.

thockin · 2019-01-04T22:44:03Z

strings/escape.go

+// EscapePluginName converts a plugin name in the format
+// vendor/pluginname into a proper ondisk vendor~pluginname plugin directory
+// format.
+func EscapePluginName(in string) string {


I swear I made this comment before, but can't find it. The Escape/UnescapePluginName funcs are totally redundant. Can we get rid of them?

Dropped! (Escape|Unescape)PluginName

thockin · 2019-01-04T22:45:18Z

We can manually merge once comments are resolved

Change-Id: I8b213a52dbd96c510e4f7173324e198f4caf55fd

dims · 2019-01-24T12:16:22Z

@thockin this is ready, please take a look

thockin

/lgtm
/approve

k8s-ci-robot · 2019-01-25T22:20:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mcrute, 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~~ [dims,thockin]

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

thockin · 2019-01-25T22:21:23Z

Manual merge because of CLA of multiple commits

kubernetes#55 was merged shortly after kubernetes#68 and then brought back glog, which now breaks vendoring of k8s.io/utils (can't have both glog and klog defining the same command line flags). Instead of replacing the logging with klog, logging gets removed entirely. The rationale is that the log output is likely to be more useful and/or readable if it is done by the caller. This is also a first step towards removing the klog dependency from k8s.io/utils (kubernetes#68 (comment)).

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 19, 2018

k8s-ci-robot requested review from dashpole and deads2k October 19, 2018 11:17

k8s-ci-robot assigned thockin Oct 19, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 19, 2018

dims changed the title ~~Move pkg util strings from kubernetes kubernetes~~ Move pkg util strings/keymutex from kubernetes kubernetes Oct 19, 2018

mcrute reviewed Oct 19, 2018

View reviewed changes

k8s-ci-robot assigned mcrute Oct 19, 2018

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

thockin reviewed Oct 19, 2018

View reviewed changes

dims force-pushed the move-pkg-util-strings-from-kubernetes-kubernetes branch from ad8dbf5 to 08cbc5b Compare October 21, 2018 23:47

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

saad-ali and others added 14 commits October 22, 2018 15:44

Fix GCE Cloudprovider waitForOp bug and make GCE attach/detach atomic…

0110028

… operations.

Add strings pkg to hold strings utils

d2fe06d

Replace funtions with strings pkg Fix all references and rename the pkg

fixed spelling typo

96c2c2b

Spelling fixes inspired by github.com/client9/misspell

8620da4

Increase coverage in pkg/util/strings to 100%

268d9e1

Move linedelimiter to it's own pkg

7d203d6

Refactoring resource container

Run goimports

0095d8f

autogenerated

97a3a81

Correct the article in generated documents

7554c42

For example: "a Ingress" > "an Ingress"

autoupdate BUILD files

7e61e68

autogenerated

6bb1ef6

snip pkg/util/strings dependency

034b5d4

Enable auto-generating sources rules

1883524

ixdy and others added 5 commits October 22, 2018 15:44

Autogenerate BUILD files

19ea2b7

Autogenerated: hack/update-bazel.sh

4d41905

Using a fixed set of locks, then we don't need to free unused locks

fc945b6

anymore. See kubernetes/kubernetes/pull/66442 for discussions.

fix golint errors in pkg/util/strings

58f4cf1

Signed-off-by: Christian Schlotter <[email protected]>

Move directories to the right spot

fa5daa7

Change-Id: I33c28cb65b4915b5f037c93bf62fda3f5b4c804a

dims force-pushed the move-pkg-util-strings-from-kubernetes-kubernetes branch from 08cbc5b to ddab25b Compare October 22, 2018 19:44

dims force-pushed the move-pkg-util-strings-from-kubernetes-kubernetes branch from ddab25b to 41bae72 Compare October 26, 2018 14:41

dims closed this Dec 4, 2018

thockin mentioned this pull request Dec 6, 2018

move kubernetes/kubernetes/tree/master/pkg/util/keymutex into utils #62

Closed

dims reopened this Dec 6, 2018

thockin reviewed Jan 4, 2019

View reviewed changes

pohly mentioned this pull request Jan 23, 2019

Moves hostpath driver from kubernetes-csi/drivers monorepo kubernetes-csi/csi-driver-host-path#1

Merged

dims mentioned this pull request Jan 23, 2019

CLAbot is lacking logs cncf/foundation#26

Closed

Drop (Escape|Unescape)PluginName

2b39f60

Change-Id: I8b213a52dbd96c510e4f7173324e198f4caf55fd

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2019

thockin reviewed Jan 25, 2019

View reviewed changes

thockin merged commit fe7be10 into kubernetes:master Jan 25, 2019

pohly mentioned this pull request Jan 28, 2019

keymutex: remove glog usage #74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pkg util strings/keymutex from kubernetes kubernetes #55

Move pkg util strings/keymutex from kubernetes kubernetes #55

dims commented Oct 19, 2018 •

edited by thockin

Loading

dims commented Oct 19, 2018

k8s-ci-robot commented Oct 19, 2018

dims commented Oct 19, 2018

mcrute left a comment

mcrute Oct 19, 2018

mcrute Oct 19, 2018

dims Oct 19, 2018

mcrute Oct 19, 2018

thockin Oct 19, 2018

mcrute commented Oct 19, 2018

thockin Oct 19, 2018

thockin Oct 19, 2018

dims Oct 21, 2018

dims Oct 21, 2018

dims Oct 21, 2018

dims Oct 22, 2018

dims Oct 22, 2018

thockin Oct 19, 2018

dims Oct 21, 2018

dims commented Oct 26, 2018

dims commented Nov 2, 2018

thockin commented Dec 6, 2018

dims commented Dec 6, 2018

pohly commented Dec 6, 2018

thockin Jan 4, 2019

dims Jan 23, 2019

thockin commented Jan 4, 2019

dims commented Jan 24, 2019

thockin left a comment

k8s-ci-robot commented Jan 25, 2019

thockin commented Jan 25, 2019

Move pkg util strings/keymutex from kubernetes kubernetes #55

Move pkg util strings/keymutex from kubernetes kubernetes #55

Conversation

dims commented Oct 19, 2018 • edited by thockin Loading

dims commented Oct 19, 2018

k8s-ci-robot commented Oct 19, 2018

dims commented Oct 19, 2018

mcrute left a 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

mcrute commented Oct 19, 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

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

dims commented Oct 26, 2018

dims commented Nov 2, 2018

thockin commented Dec 6, 2018

dims commented Dec 6, 2018

pohly commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Jan 4, 2019

dims commented Jan 24, 2019

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 25, 2019

thockin commented Jan 25, 2019

dims commented Oct 19, 2018 •

edited by thockin

Loading