-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Update Travis CI to use most current k8s server version #665
Update Travis CI to use most current k8s server version #665
Conversation
/cc @roycaihw |
@micw523: ¯\_(ツ)_/¯ In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
scripts/kube-init.sh
Outdated
sudo minikube start --vm-driver=$MINIKUBE_DRIVER --bootstrapper=kubeadm --kubernetes-version=$K8S_VERSION --logtostderr | ||
|
||
# Make sure minikube information is not outdated | ||
sudo minikube update-context |
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.
Why do we need to run minikube update-context
? I assume it modifies the kubeconfig file to point to the minikube cluster and I'd expect minikube start
does that for us already
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.
It's effectively a safeguard: https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/update-context.go#L36
scripts/kube-init.sh
Outdated
# Make sure minikube information is not outdated | ||
sudo minikube update-context | ||
|
||
export MINIKUBE_OK="false" |
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.
Let's not export this variable and make it local
scripts/kube-init.sh
Outdated
@@ -30,6 +30,11 @@ trap "clean_exit" EXIT | |||
# Switch off SE-Linux | |||
setenforce 0 | |||
|
|||
# Mount root to fix dns issues | |||
# Define $HOME since somehow this is not defined | |||
export HOME=/home/travis |
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.
This is unexpected. The travis doc mentioned that default the environment variables are available and HOME=/home/travis (do not depend on this value)
.
If the environment variable is truly missed, I'm wondering if ~
like the previous implementation works 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.
I think $HOME works in 14.04, but somehow it’s not there in the 16.04 env. I was super surprised as well. ~ doesn’t work since we have to define MINIKUBE_HOME and it does not like itself to be defined ~
# Mount root to fix dns issues | ||
# Define $HOME since somehow this is not defined | ||
export HOME=/home/travis | ||
sudo mount --make-rshared / |
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.
Which dns issue does this solve?
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.
Frankly I am not sure. I’ve seen others using it and we had disable dns in our previous script. I think kubedns
sometimes does not like the CI environments.
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.
cc @dims |
LGTM. Please squash the commits before merging |
7f199e0
to
e639053
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: micw523, roycaihw 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 |
This PR corrects behavior reported in #660 that causes e2e tests which uses API introduced after 1.7.0 to fail.
Changes:
/assign @roycaihw