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.

Sign up for GitHub

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

Jump to bottom

Remove ExtraArgs kubeadm preflight check #63673

Merged
merged 1 commit into from
May 11, 2018
Merged

Remove ExtraArgs kubeadm preflight check #63673

merged 1 commit into from
May 11, 2018

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented May 10, 2018 •
edited
Loading

This check pulled in a number of dependencies that bloated the dep graph.

The feature itself was not worth an extra 500 dependencies so we decided
to remove the feature.

Closes kubernetes/kubeadm#497

Signed-off-by: Chuck Ha ha.chuck@gmail.com

What this PR does / why we need it:
This PR removes a check that was pulling in a lot of external dependencies. We decided the check was not worth the extra dependencies.

Special notes for your reviewer:
We might want to keep the first part of the check and only delete the second part, but it was easier to delete the whole thing.

Release note:

Removes a preflight check for kubeadm that validated custom kube-apiserver, kube-controller-manager and kube-scheduler arguments.

@chuckha
Remove ExtraArgs kubeadm preflight check
9f21f5d 
This check pulled in a number of dpendencies that bloated the dep graph.

The feature itself was not worth an extra 500 dependencies so we decided
to remove the feature.

Closes kubernetes/kubeadm#497

Signed-off-by: Chuck Ha <ha.chuck@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 10, 2018
@k8s-ci-robot k8s-ci-robot requested review from kad and xiangpengzhao May 10, 2018 14:55
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 10, 2018
@dims
Copy link
Member

dims commented May 10, 2018

i was trying luxas' suggestion out and ended up with this exact change :) LGTM 👍

@dims
Copy link
Member

dims commented May 10, 2018

we'll need a release note as we are dropping some user facing functionality

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 10, 2018
@dims
Copy link
Member

dims commented May 10, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 10, 2018
@kad
Copy link
Member

kad commented May 10, 2018

@chuckha fix the test failures, otherwise lgtm.

@chuckha
Copy link
Contributor Author

chuckha commented May 10, 2018

/test pull-kubernetes-integration

@chuckha
Copy link
Contributor Author

chuckha commented May 10, 2018

/test pull-kubernetes-verify

@chuckha
Copy link
Contributor Author

chuckha commented May 10, 2018

/test pull-kubernetes-integration

@chuckha
Copy link
Contributor Author

chuckha commented May 10, 2018

/test pull-kubernetes-verify

@kad
Copy link
Member

kad commented May 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2018
@dims
Copy link
Member

dims commented May 10, 2018

/assign @luxas
/assign @timothysc

Tim, Lucas, this is ready!

/lgtm

timothysc
timothysc approved these changes May 11, 2018
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, dims, kad, 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2018
@k8s-github-robot
Copy link

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

@timothysc
Copy link
Member

/test pull-kubernetes-integration

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 63673, 63712). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 6f182a1 into kubernetes:master May 11, 2018
@luxas
Copy link
Member

luxas commented May 11, 2018

Thanks @chuckha! This LGTM as well 👍

This was referenced May 12, 2018
Closed
Merged
k8s-github-robot pushed a commit that referenced this pull request May 12, 2018
Merge pull request #63743 from liggitt/e2e-auth-plugins
163cddf 
Automatic merge from submit-queue. 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>.

register client-go auth plugins for e2e

e2e depends on use of the gcp client-go auth provider, but did not explicitly register the auth provider. it indirectly imported the plugins via a roundabout chain (see #63731 (comment)) which broke when those imports were trimmed in #63673

if e2e requires these auth plugins, it should include them explicitly in the top-level test package

fixes #63731

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothysc timothysc timothysc approved these changes

@kad kad Awaiting requested review from kad

@xiangpengzhao xiangpengzhao Awaiting requested review from xiangpengzhao

Assignees

@dims dims

@kad kad

@timothysc timothysc

@luxas luxas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chuckha @dims @kad @k8s-ci-robot @k8s-github-robot @timothysc @luxas