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

Fix docs - enabling RBAC for minikube #862

Merged
merged 2 commits into from Mar 29, 2018

Conversation

jwerak
Copy link
Contributor

@jwerak jwerak commented Mar 28, 2018

Describe what this PR does and why we need it:

Updates documentation for starting minikube:

  • enabling RBAC for minikube
  • removing specified version (minikube has only 1.9.0 or latest, current 1.9.3 didn't work)

Changes proposed in this pull request

only docs changes above

Does this PR depend on another PR (Use this to track when PRs should be merged)

no

Which issue this PR fixes (This will close that issue when PR gets merged)

none

also remove `kubernetes-version` to use always latest k8s
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 28, 2018
@djzager djzager requested a review from mhrivnak March 28, 2018 12:28
@djzager
Copy link
Member

djzager commented Mar 28, 2018

Thank you for the submission @jwerak .

@mhrivnak
Copy link
Member

Hold on with this. I'll explain shortly.

Copy link
Member

@mhrivnak mhrivnak left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

I suspect you were using the localkube bootstrapper. It has fallen out of favor and is being deprecated in favor of kubeadm. The instructions in the README work well with kubeadm but won't work with localkube.

You probably hit the same issue as this person: kubernetes/minikube#2040

When using kubeadm, RBAC is on by default. To make matters confusing, kubeadm takes different options to --extra-config, and it fails if you try to enable RBAC the way you would with localkube.

kubeadm does allow you to specify any recent version of kubernetes, unlike localkube.

In sum, I think if there is an improvement to make for this README, it would be to clarify that kubeadm should be used as the bootstrapper.

@djzager
Copy link
Member

djzager commented Mar 28, 2018

@mhrivnak Do you have a link to how to use the kubeadm bootstrapper?

@djzager
Copy link
Member

djzager commented Mar 28, 2018

Found it 😎 , minikube profile --help

@jwerak
Copy link
Contributor Author

jwerak commented Mar 28, 2018

@mhrivnak Thanks, I've had no idea about custom bootstrap in minikube, good to know, thanks 👍

I've tested following is working for me now:

minikube start --bootstrapper kubeadm
# AND 
minikube start --bootstrapper kubeadm --kubernetes-version v1.9.4

Should I keep the version with version or without?

@mhrivnak
Copy link
Member

Should I keep the version with version or without?

I like making it clear that setting the version is an option, and I recommend doing so as a normal practice. minikube makes it easy to iterate on standing up and tearing down a cluster. You'd hate to suddenly start getting 1.10 instead of 1.9, which changes something you depended on; you might not realize that a version change happened at all, and if it did break something, once you figured out that a version change was responsible, you might not know which version you were running before.

TL;DR it seems worth setting the version explicitly to get a consistent and reliable experience. But maybe that should be called out in the documentation more directly.

Does that make sense? What do you think?

@jwerak
Copy link
Contributor Author

jwerak commented Mar 29, 2018

@mhrivnak consistent and reliable makes sense to me too, new commit has landed.

README.md Outdated
@@ -217,5 +217,5 @@ make help
```

# License

git@github.com:prgcont/workshop-ASB.git
Copy link
Member

Choose a reason for hiding this comment

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

Is this line intended to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups, little advertisement can't hurt they say.
Sorry for that.
It's gone now.

@jmrodri jmrodri self-requested a review March 29, 2018 13:33
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

+1 to @mhrivnak comments

@mhrivnak mhrivnak merged commit 8d5e774 into openshift:master Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants