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] Pass features gates to components #2037

Merged
merged 2 commits into from
Oct 16, 2017

Conversation

r2d4
Copy link
Contributor

@r2d4 r2d4 commented Oct 6, 2017

Passes feature gates properly to apiserver, scheduler, controller-manager, and kubelet for the kubeadm bootstrapper.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 6, 2017
@r2d4 r2d4 force-pushed the kubeadm-feature-gates branch 2 times, most recently from 539e3ef to 11c9318 Compare October 6, 2017 20:23
@codecov-io
Copy link

codecov-io commented Oct 6, 2017

Codecov Report

Merging #2037 into master will increase coverage by 0.47%.
The diff coverage is 81.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2037      +/-   ##
==========================================
+ Coverage   28.59%   29.06%   +0.47%     
==========================================
  Files          80       81       +1     
  Lines        5243     5288      +45     
==========================================
+ Hits         1499     1537      +38     
- Misses       3551     3558       +7     
  Partials      193      193
Impacted Files Coverage Δ
pkg/minikube/bootstrapper/kubeadm/templates.go 100% <100%> (ø)
pkg/minikube/bootstrapper/kubeadm/kubeadm.go 16.07% <63.63%> (+4.16%) ⬆️
pkg/minikube/bootstrapper/kubeadm/versions.go 61.64% <88.88%> (+7.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3acb2a1...967913b. Read the comment docs.

Copy link
Contributor

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

Looks basically fine, the tests seem a little verbose though.

api:
advertiseAddress: 192.168.1.101
bindPort: 8443
kubernetesVersion: v1.8.0-alpha.0
Copy link
Contributor

Choose a reason for hiding this comment

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

The hardcoded versions here seem like they'd make these tests pretty brittle, can we get away without having this much boilerplate checked in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The versions are passed in on line 167 so they won't change. The "expectedCfg" is generated entirely from the bootstrapper.KubernetesConfig object

for k := range componentToKubeadmConfigKey {
keys = append(keys, k)
}
sort.Strings(keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this sort? Keys should be unique, and it looks like we collect the result sinto another map (extraConfig).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extraConfig is only an intermediary map that eventually gets passed to kubeadmExtraArgs, a slice.

I think I did this because a slice of []ComponentExtraArgs is passed to the template

type ComponentExtraArgs struct {
	Component string
	Options   map[string]string
}

For tests, we want to write the slice in alphabetical order according to components, and then for each component, write the options in alphabetical order.

I agree this whole construction is a bit suspect, so any refactoring ideas would be appreciated.

@r2d4 r2d4 merged commit 6bb07b3 into kubernetes:master Oct 16, 2017
@r2d4 r2d4 deleted the kubeadm-feature-gates branch October 16, 2017 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants