Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

moving ShuffleStrings to pkg/proxy #81605

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

yastij
Copy link
Member

@yastij yastij commented Aug 19, 2019

Signed-off-by: Yassine TIJANI ytijani@vmware.com

What type of PR is this?

/kind cleanup
/sig api-machinery

What this PR does / why we need it: this moves the slice package to apimachinery repository in order to be able to move some controllers out k/k

Which issue(s) this PR fixes: part of #81172

Special notes for your reviewer:

/assign @liggitt @dims

Does this PR introduce a user-facing change?:

NONE

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


@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 19, 2019
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 19, 2019
@yastij
Copy link
Member Author

yastij commented Aug 19, 2019

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 19, 2019
@neolit123
Copy link
Member

neolit123 commented Aug 19, 2019

/assign @andrewsykim
as per #81172

@yastij
there seem to be BUILD related problems in the CI logs, would ./hack/update-bazel.sh solve that?

+1 on the change.

@andrewsykim
Copy link
Member

pkg/util/slice seems mostly specific to strings, wondering if it makes more sense to add it to k8s.io/utils/strings?

@dims dims removed their assignment Aug 20, 2019
@yastij
Copy link
Member Author

yastij commented Aug 21, 2019

@andrewsykim - this would pull util/rand's apimachinery package into k/utils

@andrewsykim
Copy link
Member

I think k8s.io/apimachinery/pkg/util/rand could also belong in k8s.io/utils but will let someone from API Machinery comment on that one.

@yastij
Copy link
Member Author

yastij commented Aug 21, 2019

cc @kubernetes/sig-api-machinery-pr-reviews

@liggitt
Copy link
Member

liggitt commented Aug 21, 2019

the ShuffleStrings method is only used by pkg/proxy/.... I'd probably move that one out to the proxy package, drop the utilrand dependency, and move the rest to k/utils with no non-stdlib dependencies

@yastij
Copy link
Member Author

yastij commented Aug 21, 2019

sgtm

@yastij yastij changed the title moving the slice package to apimachinery moving ShuffleStrings to pkg/proxy Aug 21, 2019
@yastij yastij force-pushed the move-slice-to-apimachinery branch 2 times, most recently from 31416a4 to bea90bb Compare August 21, 2019 18:47
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 21, 2019
Signed-off-by: Yassine TIJANI <ytijani@vmware.com>
@liggitt
Copy link
Member

liggitt commented Aug 21, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, yastij

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 22, 2019

@yastij: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-storage-slow 57686acf7e06ad97a59ae89694f81a80e2183511 link /test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-e2e-gce-csi-serial 57686acf7e06ad97a59ae89694f81a80e2183511 link /test pull-kubernetes-e2e-gce-csi-serial

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@yastij
Copy link
Member Author

yastij commented Aug 22, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 9c16209 into kubernetes:master Aug 22, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 22, 2019
@thockin
Copy link
Member

thockin commented Aug 22, 2019

I'm not convinced that any of the logic here rises to the level of a new pkg in k/utils. It's all just trivial stuff.

@liggitt
Copy link
Member

liggitt commented Aug 22, 2019

I'm not convinced that any of the logic here rises to the level of a new pkg in k/utils. It's all just trivial stuff.

that's fair, too.

@yastij yastij deleted the move-slice-to-apimachinery branch August 23, 2019 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants