-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use the kubeadmin for minikube #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the contribution!! 👍 This looks and seems to work great!
I have researched about this earlier and I left a comment with some details about what I found out so far.
As mentioned, I don't have strong opinions on this problem. I would love to see this merged, as with kubeadm
bootstrapper we could use the latest Kubernetes version.
Maybe I would just add a note that xenial
is beta, so users know if some problems appear.
Of course, I will leave it on @lilic to decide is it okay to merge this. :)
Thank you so much!
@@ -1,5 +1,7 @@ | |||
sudo: required | |||
|
|||
# We need the systemd for the kubeadm and it's default from 16.04+ | |||
dist: xenial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to use dist: xenial
?
I was researching about it, however, I was not able to find any fresh information about that. The newest information I found is about 3 months old.
16.04 (Xenial) is still not officially supported by Travis-CI. The image is available, but it can disappear at any time and it is not guaranteed that builds will work.
Some references:
- The Travis-CI documentation states only that Trusty, Precise (note that 12.04 has reached EOL) and macOS. However, it doesn't state when the document has been updated last time.
- travis-ci/travis-ci#5821, the old one, but has some context about the issues. At that time (2016 and 2017), it was not know when is Xenial going to get published.
- travis-ci/travis-ci#7260. The last comment is from 3-4 months ago, but no update from Travis team, and the issue got locked after that. Also, there's a comment stating that it may or may not work, and that it's not supported.
- This PR (travis-ci/travis-yml#16) added Xenial support to Travis-CI, but marked as Edge feature (it is still marked as an edge feature). Following the mentioned issues from that one, there are reported problems from April that builds were randomly failing.
However, some of the problems seems solved. The build details are now correct, and reports Xenial instead of Trusty:
Operating System Details
Distributor ID: Ubuntu
Description: Ubuntu 16.04.5 LTS
Release: 16.04
Codename: xenial
I don't have strong opinions on this one. Moreover, I would really love this to get merged! The localkube
bootstrap is deprecated by the Minikube team, and switching to kubeadm
would help a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xmudrii Thanks your rapid reply.
As I know, it's absolute not safe to use xenial here (It's beta feature and you already know).
In my opinion, that's a trade off and I agree with you that we should use some documents to tell the developers xenial
is a beta feature and let them choose what kind of the combination they want to use.
- kubeadm + beta:xenial
- localkube + stable ubuntu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwchiu I agree with you.
What we can do is to create a new branch, and then merge the PR against that branch. Then update the README files in all three branches to include a short comparison between each other and note what's the recommended solution.
In that case, we would have examples for all three cases.
I think you can't create a new branch, but if that makes sense and Lili agrees, Lili or I can do it for you. Then, you will be able to update the PR to use the new branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwchiu I created a new branch https://github.com/LiliC/travis-minikube/tree/kubeadm/xenial feel free to change the PR to be merged to that. And we can after that do a followup with the README updated. Thanks! Sorry for not getting back to your earlier!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilic
Thanks your help 👍
Since the base branch of this PR is different of kubeadm/xenial
branch, there're some conflicts and should I open another PR to handle the kubeadm for kubeadm/xenial
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwchiu Looks like you're based on the 1.10 branch. I will recreate the kubeadm/xenial
, so you can merge without creating a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hwchiu I've created a new kubeadm/xenial
branch based of 1.10 branch. Try again now and let me know does it work for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks your help and I have change the target branch to kubeadm/xenial
@hwchiu could you please re-trigger the travis build to see if it still works today? |
@fabiand Done. The build is passing as intended. If you have |
The |
Thanks. It was really the language, my RP is here:
https://github.com/fabiand/common-templates/pull/1
(not as embarassing hacky as before ;) )
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I will leave it on @lilic to decide is it okay to merge this. :)
lgtm and I agree with creating the branches! Thank you for this, both of you!
@hwchiu Thanks again! Would you like to open a PR to point to this branch in the master README file? |
Yes, I can do that. |
FYI I have a similar repo now: https://github.com/fabiand/traviskube/ It tests with minikube, oc cluster, and I'm also working on minishift (which internally is also using oc cluster, but is still doing it's distinct setup). |
systemd
, we should use theubuntu 16.04
as the OS.localkube
tokubeadm