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

Prune literally all kube-apiserver internal client #71136

Conversation

yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Nov 16, 2018

i dont think this will get into v1.13, so for now, it's just experimental.

/sig api-machienery
/kind cleanup

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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. labels Nov 16, 2018
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 16, 2018
@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch from 9941d97 to 2d8b6ab Compare November 16, 2018 16:42
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 16, 2018
@yue9944882 yue9944882 changed the title Prune literally all k/k internal client Prune literally all kube-apiserver internal client Nov 16, 2018
@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch 2 times, most recently from d36a8ac to fac5503 Compare November 16, 2018 16:55
@@ -46,8 +47,7 @@ import (
"k8s.io/kubernetes/pkg/api/legacyscheme"
"k8s.io/kubernetes/pkg/api/testapi"
api "k8s.io/kubernetes/pkg/apis/core"
rbacapi "k8s.io/kubernetes/pkg/apis/rbac"
Copy link
Member Author

Choose a reason for hiding this comment

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

@deads2k 😺 i guess it should have been done in #63967

@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch from fac5503 to 15d899b Compare November 16, 2018 17:26
@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Nov 16, 2018
@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch 3 times, most recently from b43003a to e2e985b Compare November 16, 2018 17:51
@lavalamp
Copy link
Member

:) :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2018
@jennybuckley
Copy link

/cc @yliaog

@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch 3 times, most recently from bcc679c to 16b941d Compare April 8, 2019 13:16
@yue9944882
Copy link
Member Author

/test pull-kubernetes-integration

@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch 3 times, most recently from 2bfe523 to 27e0fe2 Compare April 9, 2019 10:45
@yue9944882 yue9944882 force-pushed the chore/prune-all-kk-internal-clientset branch from 27e0fe2 to 8f601d3 Compare April 9, 2019 13:44
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 9, 2019

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gke e2e985b76f1684c0fe8473fb713786faf7c18010 link /test pull-kubernetes-e2e-gke
pull-kubernetes-e2e-kops-aws e2e985b76f1684c0fe8473fb713786faf7c18010 link /test pull-kubernetes-e2e-kops-aws

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.

@yue9944882
Copy link
Member Author

/retest

@@ -106,7 +107,12 @@ func AddSystemPriorityClasses() genericapiserver.PostStartHookFunc {
_, err := schedClientSet.PriorityClasses().Get(pc.Name, metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
_, err := schedClientSet.PriorityClasses().Create(pc)
// TODO: Remove this explicit conversion after scheduling api move to v1
Copy link
Member

Choose a reason for hiding this comment

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

these are v1 in #73557

scheduling.SystemPriorityClasses() should probably return versioned objects (and the method might need to move to a different package to be able to do that)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixing in this thread #76362

@@ -30,12 +31,12 @@ import (
"k8s.io/apiserver/pkg/registry/rest"
genericapiserver "k8s.io/apiserver/pkg/server"
serverstorage "k8s.io/apiserver/pkg/server/storage"
schedulingclient "k8s.io/client-go/kubernetes/typed/scheduling/v1beta1"
Copy link
Member

Choose a reason for hiding this comment

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

use the v1 client

Copy link
Member Author

Choose a reason for hiding this comment

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

fixing in this thread #76362

ditto

@@ -71,7 +71,6 @@ INTERNAL_DIRS_CSV=$(IFS=',';echo "${INTERNAL_DIRS[*]// /,}";IFS=$)

# This can be called with one flag, --verify-only, so it works for both the
# update- and verify- scripts.
${clientgen} --input-base="k8s.io/kubernetes/pkg/apis" --input="${INTERNAL_DIRS_CSV}" "$@"
Copy link
Member

Choose a reason for hiding this comment

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

drop INTERNAL_DIRS_CSV?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixing here #76363

@liggitt
Copy link
Member

liggitt commented Apr 9, 2019

a couple nits, but since this is a rebase magnet, we can take those in follow-up PRs

/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 Apr 9, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Apr 9, 2019
@yliaog
Copy link
Contributor

yliaog commented Apr 9, 2019

good to see this done.

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit e2091b7 into kubernetes:master Apr 9, 2019
vdemeester pushed a commit to vdemeester/kubernetes that referenced this pull request Apr 10, 2019
…rune-internal-client-codegen-scripts

Follow-up of kubernetes#71136: Clean up unused vars in codegen scripts
k8s-ci-robot added a commit that referenced this pull request Oct 24, 2019
…nal-client-scheduling-v1-api

Follow-up of #71136: Switch system priority class usage to versioned (v1) api
wozniakjan added a commit to wozniakjan/autoscaler that referenced this pull request Oct 25, 2019
wozniakjan added a commit to wozniakjan/autoscaler that referenced this pull request Oct 25, 2019
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/apiserver 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants