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

Make it possible to use custom GCS with environment variables #2368

Closed
wants to merge 2 commits into from

Conversation

afbjorklund
Copy link
Collaborator

I know that "localkube" is deprecated, and that there are no official versions of localkube since v1.8.0:
#2134

But since the list of releases is hardcoded, I can't make any custom localkube builds (of 1.8.x) either:
#2156

Plan to move to "kubeadm" eventually (1.9+), but am not ready to do it for kubernetes version 1.8

And besides, using docker-machine and localkube still works fine - unless it is hardcoded/blocked...

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 29, 2017
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 29, 2017
@afbjorklund
Copy link
Collaborator Author

CLA signed

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 29, 2017
@r2d4
Copy link
Contributor

r2d4 commented Dec 30, 2017

@afbjorklund you can already pass in a file or url to kubernetes version for localkube

--kubernetes-version=https://...
or
--kubernetes-version=file:///home/matt/localkube

@afbjorklund
Copy link
Collaborator Author

@r2d4 : that was one of the first things that I tried, but it doesn't seem to work - just got errors.

E1231 09:14:10.402682    5074 start.go:182] Error parsing version semver:  No Major.Minor.Patch elements found
Kubernetes version downgrade is not supported. Using version: v1.8.0

So using the pre-populated cache was a workaround, which worked OK (for v1.8.0, not v1.8.4)

@afbjorklund
Copy link
Collaborator Author

The previous version (0.23) just said:

Invalid Kubernetes version.
The following Kubernetes versions are available: 
	- v1.8.0
	- v1.7.5
	- v1.7.4
	...

@dlorenc
Copy link
Contributor

dlorenc commented Jan 2, 2018

@afbjorklund what string did you try passing as a flag to get that error?

@afbjorklund
Copy link
Collaborator Author

@dlorenc : Something like --kubernetes-version=file:///mnt/sda1/tmp/localkube

@r2d4
Copy link
Contributor

r2d4 commented Jan 2, 2018

I think that this was a regression in 0.23.0 but fixed at HEAD. Especially since we use the flag in this way for integration tests

https://github.com/kubernetes/minikube/blob/master/hack/jenkins/linux_integration_tests_virtualbox.sh#L32

@afbjorklund
Copy link
Collaborator Author

Am running 0.24.1, maybe should have said that.

@afbjorklund
Copy link
Collaborator Author

Hmm, git says c20ebde was included in 0.24.0 already...
Maybe I forgot the file:// before the path or something ?

Now when I retry same thing with minikube 0.24.1, it does say:
Starting local Kubernetes file:///tmp/localkube cluster...

@dlorenc
Copy link
Contributor

dlorenc commented Jan 30, 2018

What's the status here? Can this be closed?

@afbjorklund
Copy link
Collaborator Author

afbjorklund commented Jan 30, 2018

@dlorenc : I guess that if you wanted to run e.g. 1.8.7 or 1.9.2 that you would "have" to use something like this since the default list of localkubes is still hardcoded to 1.8.0 and 1.9.0.

But using an URL seems to be working (as a workaround), even if it spews some "semver" errors when trying to parse the URL. Seems to be choking on the first path of the url/path ?

Error parsing version semver: Invalid character(s) found in major number "file:///home/anders/"

But I suppose we could patch it downstream, so you can just close it if you like.

@afbjorklund
Copy link
Collaborator Author

It would be nice if this could be merged, since the syntax is much nicer and having validation is good...
It is not supposed to break anything, if you want the defaults and don't set the environment variables ?

MINIKUBE_LOCALKUBE_GCS=https://storage.googleapis.com/minikube/k8s_releases.json
MINIKUBE_LOCALKUBE_URL=https://storage.googleapis.com/minikube/k8sReleases/

Add environment variables, for alternative locations:

$MINIKUBE_LOCALKUBE_GCS
https://storage.googleapis.com/minikube/k8s_releases.json

$MINIKUBE_LOCALKUBE_URL
https://storage.googleapis.com/minikube/k8sReleases/

Since minikube is no longer making localkube releases.

Signed-off-by: Anders F Björklund <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afbjorklund
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: dlorenc

Assign the PR to them by writing /assign @dlorenc in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dlorenc
Copy link
Contributor

dlorenc commented Jun 5, 2018

Is this still needed? Localkube is on the way out. Please reopen if you think it should be included still.

@dlorenc dlorenc closed this Jun 5, 2018
@afbjorklund
Copy link
Collaborator Author

It was used for previous kubernetes releases, but like you say it is not useful without localkube.
So I guess I will just keep the patch downstream, and then switch to using kubeadm instead...

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants