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

kubeadm: allow kubeadm init to read config from file #34501

Merged
merged 1 commit into from
Oct 13, 2016
Merged

kubeadm: allow kubeadm init to read config from file #34501

merged 1 commit into from
Oct 13, 2016

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Oct 10, 2016 •
edited by k8s-oncall

@kubernetes/sig-cluster-lifecycle

This change is Reviewable

@mikedanese mikedanese self-assigned this Oct 10, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 10, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 227a38a. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 227a38a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 10, 2016
luxas
luxas reviewed Oct 11, 2016
View reviewed changes
cmd/kubeadm/app/cmd/init.go
@@ -87,6 +97,8 @@ func NewCmdInit(out io.Writer) *cobra.Command {
`Choose a specific Kubernetes version for the control plane`,
)

cmd.PersistentFlags().StringVar(&cfgPath, "config", "", "Path to kubeadm config file")
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this in the API?

Copy link
Member

Choose a reason for hiding this comment

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

I think that it wouldn't be of much use there really.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't really make sense to put in the api. It's a path to an API object.

Copy link
Member

Choose a reason for hiding this comment

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

Ok for me

errordeveloper
errordeveloper approved these changes Oct 11, 2016
View reviewed changes
Copy link
Member

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

LGTM, few minor nits...

cmd/kubeadm/app/cmd/init.go
@@ -87,6 +97,8 @@ func NewCmdInit(out io.Writer) *cobra.Command {
`Choose a specific Kubernetes version for the control plane`,
)

cmd.PersistentFlags().StringVar(&cfgPath, "config", "", "Path to kubeadm config file")
Copy link
Member

Choose a reason for hiding this comment

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

I think that it wouldn't be of much use there really.

cmd/kubeadm/app/cmd/init.go
}
i, err := NewInit(cfgPath, cfg)
check(err)
check(fmt.Errorf("<cmd/init> %v", i.Run(out)))
Copy link
Member

@errordeveloper errordeveloper Oct 11, 2016 •
edited

Choose a reason for hiding this comment

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

I don't quite understand what is this last line is for...

Copy link
Contributor

@pires pires Oct 11, 2016 •
edited

Choose a reason for hiding this comment

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

I read it this way:

  • First check is for errors in NewInit, e.g. can't read configuration file.
  • Second check is for the actual execution of the init process (Init.Run(...)).

Copy link
Member Author

@mikedanese mikedanese Oct 11, 2016 •
edited

Choose a reason for hiding this comment

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

I should have omitted the fmt.Errorf("<cmd/init> %v", ) on the last line.

cmd/kubeadm/app/cmd/init.go
if cfgPath != "" {
b, err := ioutil.ReadFile(cfgPath)
if err != nil {
return nil, fmt.Errorf("unable to read config from %q: %v", cfgPath, err)
Copy link
Member

@errordeveloper errordeveloper Oct 11, 2016 •
edited

Choose a reason for hiding this comment

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

We've had [%v] in other places, which sort of implies "you can read this, but it might not be very meaningful to you". I know that : %v is a wider convention, I'd be happy to switch if you insist, but I'm not sure it's actually a great convention... Happy to discuss in another thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will stick with [%v] then. We can revisit if we must.

pires
pires approved these changes Oct 11, 2016
View reviewed changes
Copy link
Contributor

@pires pires left a comment

Choose a reason for hiding this comment

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

LGTM.

@mikedanese mikedanese removed their assignment Oct 11, 2016
@mikedanese
Copy link
Member Author

@errordeveloper PTAL.

@luxas
Copy link
Member

luxas commented Oct 11, 2016

LGTM

@luxas
Copy link
Member

luxas commented Oct 11, 2016

@k8s-bot test this

@k8s-ci-robot
Copy link
Contributor

Bootstrap GCE e2e failed for commit 58d25e3. Full PR test history.

The magic incantation to run this job again is @k8s-bot bootstrap gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@errordeveloper
Copy link
Member

errordeveloper commented Oct 11, 2016 •
edited by mikedanese

@k8s-bot bootstrap gce e2e test this

/lgtm

errordeveloper
errordeveloper approved these changes Oct 11, 2016
View reviewed changes
Copy link
Member

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

LGTM, let's get it in!

@mikedanese mikedanese added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 11, 2016
@mikedanese
Copy link
Member Author

Thanks for the review. @errordeveloper if you comment /lgtm on prs that are assigned to you, mergebot should add lgtm label. I added it to your above comment :) let's see if it works. I'm adding do-not-merge because I want #34555 to go in before this.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 58d25e3. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

This was referenced Oct 12, 2016
Merged
Closed
@mikedanese mikedanese added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Oct 12, 2016
@mikedanese
Copy link
Member Author

@k8s-bot ok to test, issue: #IGNORE

@mikedanese
Copy link
Member Author

@kubernetes/sig-testing hmm, my e2e's aren't getting rerun.

@errordeveloper
Copy link
Member

@k8s-bot cvm gce e2e test this

@mikedanese
Copy link
Member Author

ahh instructions.

@k8s-bot cvm gce e2e test this

@errordeveloper
Copy link
Member

@k8s-bot bootstrap gce e2e test this

1 similar comment
@errordeveloper
Copy link
Member

@k8s-bot bootstrap gce e2e test this

@errordeveloper errordeveloper mentioned this pull request Oct 13, 2016
Closed
@luxas luxas added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Oct 13, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit c1986f0 into kubernetes:master Oct 13, 2016
@luxas luxas mentioned this pull request Oct 15, 2016
Closed
@pires pires mentioned this pull request Oct 15, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 15, 2016
Merge pull request #34885 from apprenda/kubeadm_join_configuration
ab14c31 
Automatic merge from submit-queue

kubeadm join: Added support for config file.

As more behavior (#34719, #34807, fix for #33641) is added to `kubeadm join`, this will be eventually very much needed. Makes sense to go in sooner rather than later.

Also references #34501 and #34884.

/cc @luxas @mikedanese
@lukemarsden lukemarsden mentioned this pull request Oct 20, 2016
Closed
This was referenced Nov 22, 2016
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luxas luxas luxas left review comments

@errordeveloper errordeveloper errordeveloper approved these changes

@pires pires pires approved these changes

Assignees

@errordeveloper errordeveloper

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. 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.

None yet
7 participants
@mikedanese @k8s-ci-robot @luxas @errordeveloper @k8s-github-robot @pires @googlebot