move componentconfig ClientConnectionConfiguration to k8s.io/apimachinery/pkg/apis/config #66058

hanxiaoshuai · 2018-07-11T08:16:41Z

What this PR does / why we need it:
ref #2354
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
ClientConnectionConfiguration now is used by KubeSchedulerConfiguration in pkg/apis/componentconfig, when KubeSchedulerConfiguration is moved to staging, ClientConnectionConfiguration should be cleaned up in pkg/apis/componentconfig.
Release note:

NONE

nikhita · 2018-07-11T08:21:00Z

/cc @luxas @sttts

hanxiaoshuai · 2018-07-11T11:19:44Z

/hold

luxas

Thanks for this PR!
Some notes: I'd expect some defaulting funcs for v1alpha1. Also, an OWNERS file should be created as in the KEP.

luxas · 2018-07-11T17:50:50Z

@rosti FYI

hanxiaoshuai · 2018-07-12T09:24:26Z

/test pull-kubernetes-bazel-test

hanxiaoshuai · 2018-07-12T09:47:19Z

@luxas Done, Thanks for your review
/assign @sttts @luxas

hanxiaoshuai · 2018-07-12T12:37:54Z

/hold cancel

luxas

LGTM after api-approvers / reviewers are added to the OWNERS file, the KEP is merged and e.g. @thockin and @sttts have also LGTM'd this

luxas · 2018-07-12T17:16:53Z

/assign @thockin

sttts · 2018-07-27T17:09:08Z

hack/.golint_failures

@@ -473,6 +473,7 @@ staging/src/k8s.io/apimachinery/pkg/api/testing/fuzzer
 staging/src/k8s.io/apimachinery/pkg/api/testing/roundtrip
 staging/src/k8s.io/apimachinery/pkg/api/validation
 staging/src/k8s.io/apimachinery/pkg/api/validation/path
+staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1


what does it take to fix golint for that package?

Because of the file register.go in the package

sttts · 2018-07-27T17:09:53Z

For a move it has surprisingly few removed lines of code.

sttts · 2018-07-27T17:14:13Z

Same here: let's create the follow-up PR which uses this. Then we see whether the approach works. Duplicating code before the cleanup at the horizon is not a good idea.

/hold

luxas · 2018-07-30T15:30:12Z

@sttts The PR that cleans this up/moves the usage is here: #66722.
I prefer having clean PRs like this so that one PR adds the net-new code and one PR cleans stuff up later if that is ok by you.
/hold cancel

thockin · 2018-07-30T17:58:48Z

staging/src/k8s.io/apimachinery/pkg/apis/config/types.go

+limitations under the License.
+*/
+
+package config


Is this the best possible name for this package? Should it be client or clientconfig or something? "config" is REALLY nondescript.

The package is generic and may hold any generic componentconfig type. I think config is the best we can do here. We may always rename later before going beta with the package structure

luxas · 2018-08-04T20:04:14Z

Can we please get a top-level approval of this to get the ball rolling so we can then cleanup the types (PR #66722) ASAP, and unblock the components' types moving (e.g. #66916).

We can always change the structure later if we feel we need to, before going beta with this, but if this is gonna be stuck forever we have no chance improving anything.

luxas · 2018-08-04T20:05:23Z

/retest

jbeda · 2018-08-05T00:01:47Z

This looks like a reasonable first step so approving to get it moving.

/approve

fejta-bot · 2018-08-05T02:15:27Z

/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.

…nery/pkg/apis/config

stewart-yu

some nil may be need to fix
and rebase auto-generated commit log. other than LGTM to me

stewart-yu · 2018-08-05T11:30:39Z

staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/types.go

+// ClientConnectionConfiguration contains details for constructing a client.
+type ClientConnectionConfiguration struct {
+	// kubeConfigFile is the path to a kubeconfig file.
+	KubeConfigFile string `json:"kubeconfig"`


should be Capital ???
The same in others place

this maybe should be kubeConfigFile, but let's leave this as-is for now.

stewart-yu · 2018-08-05T11:52:17Z

staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/doc.go

+
+// +k8s:deepcopy-gen=package
+// +k8s:conversion-gen=k8s.io/apimachinery/pkg/apis/config
+// +k8s:defaulter-gen=TypeMeta


should remove defaulter-gen?

Maybe, but this is ok for now, follows the pattern for everything else

stewart-yu · 2018-08-05T11:57:04Z

can not agree more

luxas

/lgtm

Thanks @hanxiaoshuai and @jbeda!

luxas · 2018-08-05T12:33:34Z

staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/defaults.go

+	kruntime "k8s.io/apimachinery/pkg/runtime"
+)
+
+func addDefaultingFuncs(scheme *kruntime.Scheme) error {


This is something we will discuss if we're actually gonna do, but for now this is ok

luxas · 2018-08-05T12:33:58Z

staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/defaults.go

+
+func SetDefaults_ClientConnectionConfiguration(obj *ClientConnectionConfiguration) {
+	if len(obj.ContentType) == 0 {
+		obj.ContentType = "application/vnd.kubernetes.protobuf"


We might wanna expose these values as constants in the future.

luxas · 2018-08-05T12:34:19Z

staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/doc.go

+
+// +k8s:deepcopy-gen=package
+// +k8s:conversion-gen=k8s.io/apimachinery/pkg/apis/config
+// +k8s:defaulter-gen=TypeMeta


Maybe, but this is ok for now, follows the pattern for everything else

luxas · 2018-08-05T12:35:06Z

staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/types.go

+// ClientConnectionConfiguration contains details for constructing a client.
+type ClientConnectionConfiguration struct {
+	// kubeConfigFile is the path to a kubeconfig file.
+	KubeConfigFile string `json:"kubeconfig"`


this maybe should be kubeConfigFile, but let's leave this as-is for now.

k8s-ci-robot · 2018-08-05T12:35:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hanxiaoshuai, jbeda, luxas

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:

~~hack/OWNERS~~ [jbeda]
~~staging/OWNERS~~ [jbeda]

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

k8s-github-robot · 2018-08-05T12:39:35Z

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

k8s-github-robot · 2018-08-05T13:55:02Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@sttts

Automatic merge from submit-queue (batch tested with PRs 67071, 66906, 66722, 67276, 67039). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Remove references to the structs that have moved to their own packages **What this PR does / why we need it**: Follows-up #66058 and #66059 to remove the structs that now aren't needed in `pkg/apis/componentconfig` **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: xref kubernetes/community#2354 **Special notes for your reviewer**: This PR depends on: - [x] #67090 - [x] #67149 - [x] #67159 - [x] #67207 **Only review commit 'Remove references to the structs that have moved to their own packages' please** **Release note**: ```release-note NONE ``` /kind cleanup /assign @sttts @thockin @jbeda @liggitt

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

k8s-ci-robot requested review from gmarek and ncdc July 11, 2018 08:16

k8s-ci-robot requested review from luxas and sttts July 11, 2018 08:21

hanxiaoshuai force-pushed the machcfg branch 4 times, most recently from 004c55c to 3bb8f03 Compare July 11, 2018 10:57

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2018

hanxiaoshuai force-pushed the machcfg branch from 3bb8f03 to 7f4cda0 Compare July 11, 2018 12:04

luxas reviewed Jul 11, 2018

View reviewed changes

hanxiaoshuai force-pushed the machcfg branch 2 times, most recently from 7eb0a18 to ccca459 Compare July 12, 2018 08:17

k8s-ci-robot assigned luxas and sttts Jul 12, 2018

hanxiaoshuai force-pushed the machcfg branch from ccca459 to 13ca183 Compare July 12, 2018 12:35

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2018

hanxiaoshuai force-pushed the machcfg branch from 13ca183 to f4553e7 Compare July 12, 2018 13:30

luxas approved these changes Jul 12, 2018

View reviewed changes

k8s-ci-robot assigned thockin Jul 12, 2018

k8s-ci-robot assigned jbeda Jul 27, 2018

sttts reviewed Jul 27, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2018

luxas mentioned this pull request Jul 27, 2018

Remove references to the structs that have moved to their own packages #66722

Merged

4 tasks

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2018

thockin reviewed Jul 30, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2018

hanxiaoshuai added 4 commits August 5, 2018 11:33

move componentconfig ClientConnectionConfiguration to k8s.io/apimachi…

ad1f3e6

…nery/pkg/apis/config

add generated code files

b7ce1f8

add an OWNERS file

378855e

update zz_generated.conversion.go file

1bc0f43

hanxiaoshuai force-pushed the machcfg branch from f6703dc to 1bc0f43 Compare August 5, 2018 03:40

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

stewart-yu reviewed Aug 5, 2018

View reviewed changes

luxas approved these changes Aug 5, 2018

View reviewed changes

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

k8s-github-robot merged commit 774fe16 into kubernetes:master Aug 5, 2018

stewart-yu mentioned this pull request Aug 12, 2018

Work tracker for moving ComponentConfig types to staging repos #67233

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move componentconfig ClientConnectionConfiguration to k8s.io/apimachinery/pkg/apis/config #66058

move componentconfig ClientConnectionConfiguration to k8s.io/apimachinery/pkg/apis/config #66058

hanxiaoshuai commented Jul 11, 2018 •

edited

Loading

nikhita commented Jul 11, 2018

hanxiaoshuai commented Jul 11, 2018

luxas left a comment

luxas commented Jul 11, 2018

hanxiaoshuai commented Jul 12, 2018

hanxiaoshuai commented Jul 12, 2018

hanxiaoshuai commented Jul 12, 2018

luxas left a comment

luxas commented Jul 12, 2018

sttts Jul 27, 2018

hanxiaoshuai Jul 28, 2018

sttts commented Jul 27, 2018

sttts commented Jul 27, 2018

luxas commented Jul 30, 2018

thockin Jul 30, 2018

luxas Aug 4, 2018

luxas commented Aug 4, 2018

luxas commented Aug 4, 2018

jbeda commented Aug 5, 2018

fejta-bot commented Aug 5, 2018

stewart-yu left a comment

stewart-yu Aug 5, 2018

luxas Aug 5, 2018

stewart-yu Aug 5, 2018

luxas Aug 5, 2018

stewart-yu commented Aug 5, 2018

luxas left a comment

luxas Aug 5, 2018

luxas Aug 5, 2018

luxas Aug 5, 2018

luxas Aug 5, 2018

k8s-ci-robot commented Aug 5, 2018

k8s-github-robot commented Aug 5, 2018

k8s-github-robot commented Aug 5, 2018

move componentconfig ClientConnectionConfiguration to k8s.io/apimachinery/pkg/apis/config #66058

move componentconfig ClientConnectionConfiguration to k8s.io/apimachinery/pkg/apis/config #66058

Conversation

hanxiaoshuai commented Jul 11, 2018 • edited Loading

nikhita commented Jul 11, 2018

hanxiaoshuai commented Jul 11, 2018

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 11, 2018

hanxiaoshuai commented Jul 12, 2018

hanxiaoshuai commented Jul 12, 2018

hanxiaoshuai commented Jul 12, 2018

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 27, 2018

sttts commented Jul 27, 2018

luxas commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Aug 4, 2018

luxas commented Aug 4, 2018

jbeda commented Aug 5, 2018

fejta-bot commented Aug 5, 2018

stewart-yu 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

stewart-yu commented Aug 5, 2018

luxas 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

k8s-ci-robot commented Aug 5, 2018

k8s-github-robot commented Aug 5, 2018

k8s-github-robot commented Aug 5, 2018

hanxiaoshuai commented Jul 11, 2018 •

edited

Loading