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

Move kubelet ComponentConfig external types to `k8s.io/kubelet` #67263

Merged
merged 4 commits into from Sep 2, 2018

Conversation

@luxas
Member

luxas commented Aug 10, 2018

What this PR does / why we need it:
This PR implements most of kubernetes/community#2354 for the kubelet.
The PR:

  • Moves k8s.io/kubernetes/pkg/apis/kubeletconfig as-is to k8s.io/kubernetes/pkg/apis/config as agreed
  • Moves the external types to the new staging repo k8s.io/kubelet, in the k8s.io/kubelet/config/v1beta1 package.
  • Makes k8s.io/kubernetes/pkg/apis/config/v1beta1 source the types from k8s.io/kubelet/config/v1beta1. The defaulting and conversion code is kept in this package as before.
  • All references to these packages have been updated.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref: kubernetes/community#2354

Special notes for your reviewer:

This PR depends on getting #67780 merged first.

Release note:

kubelet v1beta1 external ComponentConfig types are now available in the `k8s.io/kubelet` repo

/assign @sttts @mtaufen @liggitt

@mtaufen

This comment has been minimized.

Show comment
Hide comment
@mtaufen

mtaufen Aug 11, 2018

Contributor

We should double check for any links on the docs website that point to the things we are moving. I don't think it should be a problem (those links should always point to release branches, not master), but worth double-checking.

Contributor

mtaufen commented Aug 11, 2018

We should double check for any links on the docs website that point to the things we are moving. I don't think it should be a problem (those links should always point to release branches, not master), but worth double-checking.

@luxas

This comment has been minimized.

Show comment
Hide comment
@luxas

luxas Aug 23, 2018

Member

Split this out to #67780 as the first scoped PR.

Member

luxas commented Aug 23, 2018

Split this out to #67780 as the first scoped PR.

@dims

This comment has been minimized.

Show comment
Hide comment
@dims

dims Aug 24, 2018

Member

/uncc

Member

dims commented Aug 24, 2018

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from dims Aug 24, 2018

@luxas luxas changed the title from WIP: Move kubelet ComponentConfig external types to `k8s.io/kubelet` to Move kubelet ComponentConfig external types to `k8s.io/kubelet` Aug 30, 2018

@luxas luxas added this to the v1.12 milestone Aug 30, 2018

k8s-merge-robot added a commit that referenced this pull request Aug 30, 2018

Merge pull request #67780 from luxas/move_kubelet_config_pkg
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Move kubelet internal ComponentConfig types to `pkg/kubelet/apis/config`

**What this PR does / why we need it**:
This PR is split out from the main PR of #67263, in order to make merging each scoped piece of the puzzle easier and smoother.

This PR simply moves the `k8s.io/kubernetes/pkg/apis/kubeletconfig` as-is to `k8s.io/kubernetes/pkg/apis/config` as agreed in the KEP.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
ref: kubernetes/community#2354

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```
@kubernetes/sig-node-pr-reviews 
/assign @mtaufen @thockin @liggitt
@luxas

This comment has been minimized.

Show comment
Hide comment
@luxas

luxas Aug 30, 2018

Member

#67780 has merged, this PR is rebased and ready for review and merge in time for v1.12

Member

luxas commented Aug 30, 2018

#67780 has merged, this PR is rebased and ready for review and merge in time for v1.12

@thockin

This comment has been minimized.

Show comment
Hide comment
@thockin

thockin Aug 30, 2018

Member

/lgtm
/approve

Thanks - easy to review

Member

thockin commented Aug 30, 2018

/lgtm
/approve

Thanks - easy to review

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Aug 31, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Aug 31, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Aug 31, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Aug 31, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@@ -15,5 +15,6 @@ limitations under the License.
*/
// +k8s:deepcopy-gen=package
// TODO: This file should also enable openapi-gen like the other packages

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

why? this is not served by an apiserver.

@sttts

sttts Aug 31, 2018

Contributor

why? this is not served by an apiserver.

This comment has been minimized.

@luxas

luxas Aug 31, 2018

Member

We might consider this a bug, but the tag is needed in order for API violations to be tracked by the api/api-rules/violation_exceptions.list, which I think is something we want to do. If we get all types to be tracked by that file automatically, while not generating any extra code, fine by me but for now let's track bad stuff there for componentconfigs as well

@luxas

luxas Aug 31, 2018

Member

We might consider this a bug, but the tag is needed in order for API violations to be tracked by the api/api-rules/violation_exceptions.list, which I think is something we want to do. If we get all types to be tracked by that file automatically, while not generating any extra code, fine by me but for now let's track bad stuff there for componentconfigs as well

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

Oh really? non-blocker for me. How you like.

@sttts

sttts Aug 31, 2018

Contributor

Oh really? non-blocker for me. How you like.

This comment has been minimized.

@luxas

luxas Sep 1, 2018

Member

Gonna open an issue later about this

@luxas

luxas Sep 1, 2018

Member

Gonna open an issue later about this

@@ -21,19 +21,23 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kruntime "k8s.io/apimachinery/pkg/runtime"
kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1"
// TODO: Cut references to k8s.io/kubernetes, eventually there should be none from this package

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

👍

@sttts

sttts Aug 31, 2018

Contributor

👍

@@ -16,7 +16,9 @@ limitations under the License.
// +k8s:deepcopy-gen=package
// +k8s:conversion-gen=k8s.io/kubernetes/pkg/kubelet/apis/config
// +k8s:openapi-gen=true
// +k8s:conversion-gen-external-types=k8s.io/kubelet/config/v1beta1

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

is this necessary to point to itself?

@sttts

sttts Aug 31, 2018

Contributor

is this necessary to point to itself?

This comment has been minimized.

@luxas

luxas Aug 31, 2018

Member

it's not pointing to itself, it's pointing to the staging types from the internal k8s.io meta pkg

@luxas

luxas Aug 31, 2018

Member

it's not pointing to itself, it's pointing to the staging types from the internal k8s.io meta pkg

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

oh, not itself. nvmd.

@sttts

sttts Aug 31, 2018

Contributor

oh, not itself. nvmd.

*/
// +k8s:deepcopy-gen=package
// +k8s:openapi-gen=true

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

not sure we need this, probably we don't. This is not served.

@sttts

sttts Aug 31, 2018

Contributor

not sure we need this, probably we don't. This is not served.

This comment has been minimized.

@sttts

sttts Aug 31, 2018

Contributor

like the other comment. If this is needed, for api violations, leave it.

@sttts

sttts Aug 31, 2018

Contributor

like the other comment. If this is needed, for api violations, leave it.

This comment has been minimized.

@luxas

luxas Sep 1, 2018

Member

yeah needed for api violations for now

@luxas

luxas Sep 1, 2018

Member

yeah needed for api violations for now

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 31, 2018

Contributor

Lgtm, some nits and the README.

Contributor

sttts commented Aug 31, 2018

Lgtm, some nits and the README.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Aug 31, 2018

Contributor

/lgtm

if it is green, fix the README in a follow-up.

Contributor

sttts commented Aug 31, 2018

/lgtm

if it is green, fix the README in a follow-up.

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 31, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Aug 31, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Contributor

k8s-ci-robot commented Aug 31, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@liggitt liggitt removed their assignment Aug 31, 2018

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Aug 31, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Aug 31, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot commented Sep 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

luxas added some commits Aug 30, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 2, 2018

Contributor

New changes are detected. LGTM label has been removed.

Contributor

k8s-ci-robot commented Sep 2, 2018

New changes are detected. LGTM label has been removed.

@luxas

This comment has been minimized.

Show comment
Hide comment
@luxas

luxas Sep 2, 2018

Member

I'm re-applying the LGTM label, as I now rebased the PR (the need for a rebase was the cause of the failing tests retried by fejta-bot).
All tests are green, and I fixed the nit in the README.

Member

luxas commented Sep 2, 2018

I'm re-applying the LGTM label, as I now rebased the PR (the need for a rebase was the cause of the failing tests retried by fejta-bot).
All tests are green, and I fixed the nit in the README.

@luxas luxas added the lgtm label Sep 2, 2018

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Sep 2, 2018

Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented Sep 2, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Sep 2, 2018

Contributor

Automatic merge from submit-queue (batch tested with PRs 65566, 67959, 68029, 68017, 67263). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Contributor

k8s-merge-robot commented Sep 2, 2018

Automatic merge from submit-queue (batch tested with PRs 65566, 67959, 68029, 68017, 67263). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-merge-robot k8s-merge-robot merged commit 3a8a711 into kubernetes:master Sep 2, 2018

14 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce-100-performance
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-local-e2e-containerized Job triggered.
Details
cla/linuxfoundation luxas authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment