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

kubeadm: Introduce v1beta2 config #76710

Merged
merged 1 commit into from Apr 26, 2019

Conversation

@rosti
Copy link
Member

commented Apr 17, 2019

What type of PR is this?
/kind api-change

What this PR does / why we need it:

This change adds v1beta2 config to kubeadm. Currently this is a 1:1 copy of v1beta1.

Which issue(s) this PR fixes:

Refs kubernetes/enhancements#970 kubernetes/kubeadm#1439

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-longterm
/assign @fabriziopandini
/assign @timothysc
/assign @luxas

Does this PR introduce a user-facing change?:

Introduce the v1beta2 config format to kubeadm.
@rosti

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

/hold
For kubernetes/enhancements#969 to merge.

@rosti

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

/assign @fejta
For the hack/.golint_failures change.

@@ -3,6 +3,7 @@ cmd/kube-apiserver/app
cmd/kube-controller-manager/app
cmd/kube-proxy/app
cmd/kubeadm/app/apis/kubeadm/v1beta1
cmd/kubeadm/app/apis/kubeadm/v1beta2

This comment has been minimized.

Copy link
@fejta

fejta Apr 19, 2019

Contributor

This moves us in the wrong direction.

Please address the lint errors in your code and remove the exception on this package.

This comment has been minimized.

Copy link
@neolit123

neolit123 Apr 19, 2019

Member

my research on the problem here is the code generated by the k8s code generators do not pass the current state of go linters. they use underscores in generated function names such SetDefaults_MyFunction - e.g.
https://github.com/kubernetes/kubernetes/pull/76710/files#diff-9a2f5c3d2d4df5c049e0951f45d50133R69

this means that API namespaces need to be skipped until we fix the generator(s).

This comment has been minimized.

Copy link
@rosti

rosti Apr 22, 2019

Author Member

@neolit123 is right here. I am not aware of any way of making the defaulter gen call funcs, that don't have underscores in their name and these obviously break the linter.

This comment has been minimized.

Copy link
@fejta

fejta Apr 22, 2019

Contributor

Is there an issue to fix this in the code generators?

This comment has been minimized.

Copy link
@neolit123

neolit123 Apr 22, 2019

Member

this PR seems to fix comment related lint issues, but not the _:
#59674

this ticket has a good enough title to be re-used:
kubernetes/code-generator#30

i will re-open it and explain the _ case.

This comment has been minimized.

Copy link
@fejta

fejta Apr 25, 2019

Contributor

Thanks!

@timothysc
Copy link
Member

left a comment

/approve

I'll let @neolit123 and @fabriziopandini go over with a fine-toothed comb.

kubeadm: Introduce v1beta2 config
Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

@rosti rosti force-pushed the rosti:introduce-v1beta2 branch from 24a28e8 to 9e1ac76 Apr 25, 2019

@fejta

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

/approve
/lgtm
thanks!

@fejta

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

/lgtm cancel
Didn't review, but approving the hack change

@k8s-ci-robot k8s-ci-robot added approved and removed lgtm labels Apr 25, 2019

@neolit123
Copy link
Member

left a comment

/lgtm

//
// Migration from old kubeadm config versions
//
// Please convert your v1alpha3 configuration files to v1beta2 using the "kubeadm config migrate" command of kubeadm v1.13.x

This comment has been minimized.

Copy link
@neolit123

neolit123 Apr 25, 2019

Member

we'll need to have the doc.go as a TODO.
the search and replace is fine for now.

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Apr 26, 2019

Member

kubeadm v1.13.x can't migrate to v1.13.x, or I'm wrong?
Probably also all the conversion comment should be updated as well

@fabriziopandini
Copy link
Member

left a comment

@rosti thanks for taking lead on this effort!
/approve
/lgtm

Only two nits on docs; up to you decide if to fix in this PR or in subsequent PRs

//
// Migration from old kubeadm config versions
//
// Please convert your v1alpha3 configuration files to v1beta2 using the "kubeadm config migrate" command of kubeadm v1.13.x

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Apr 26, 2019

Member

kubeadm v1.13.x can't migrate to v1.13.x, or I'm wrong?
Probably also all the conversion comment should be updated as well

// Nevertheless, kubeadm v1.13.x will support reading from v1alpha3 version of the kubeadm config file format, but this support
// will be dropped in the v1.14 release.
//
// Basics

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini Apr 26, 2019

Member

I think that it would be nice to add a "release note" paragraph describing major changes in v1beta2.
This can be done in this PR or in subsequent PR as well

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, fejta, rosti, timothysc

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

@dixudx
dixudx approved these changes Apr 26, 2019
Copy link
Member

left a comment

/lgtm

@timothysc

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@rosti I'm ok with an /hold cancel when you are.

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 26, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 332d62a into kubernetes:master Apr 26, 2019

15 of 20 checks passed

pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-integration Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@rosti: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-integration 9e1ac76 link /test pull-kubernetes-integration

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.