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

minikube: Use localkube #31621

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
5 participants
@NeQuissimus
Member

NeQuissimus commented Nov 13, 2017

Motivation for this change

Fixes #31602

The minikube guys changed the behaviour of how they load localkube.
Since they always try to download a remote version and store it (in /nix/store for us), the remote pull did not work.
This reverts to the old behaviour that uses the bundled localkube

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

/cc @moretea

@moretea

This comment has been minimized.

Contributor

moretea commented Nov 13, 2017

This works perfectly fine.

$ rm -rf ~/.minikube
$ minikube start
Starting local Kubernetes v1.8.0+54e1cc9df cluster...
Starting VM...
Downloading Minikube ISO
 140.01 MB / 140.01 MB [============================================] 100.00% 0s
Getting VM IP address...
Moving files into cluster...
Setting up certs...
Connecting to cluster...
Setting up kubeconfig...
Starting cluster components...
Kubectl is now configured to use the cluster.
- if err != nil {
- return errors.Wrap(err, "Error updating localkube from uri")
- }
+ localkubeFile = assets.NewBinDataAsset("out/localkube", "/usr/local/bin", "localkube", "0777")

This comment has been minimized.

@copumpkin

copumpkin Nov 13, 2017

Member

How does this work?

This comment has been minimized.

@NeQuissimus

NeQuissimus Nov 14, 2017

Member

The old behaviour (we made use) of was that minikube came with a local version of localkube.
In 0.23.0 this was changed so that minikube itself is much thinner and auto-downloads localkube. This patch reverts to using the local locations again.
Technically, this command looks in three locations and requires one of them to exist. out/localkube is what will kick in for us. But I kept the other two locations to stay with the original code.

This comment has been minimized.

@copumpkin

copumpkin Nov 14, 2017

Member

Ah I just reacted negatively to the /usr/local/bin 😄 I'd probably take that out. Feels really weird to be intentionally patching in an impure location (even if unused), especially to someone who doesn't know intimately how the code works (and who runs on a platform without a working sandbox) 😄 feels like someone adjusting the expression in future might accidentally fall through to impure behavior.

This comment has been minimized.

@NeQuissimus

NeQuissimus Nov 14, 2017

Member

Makes sense, I'll take it out tomorrow morning

This comment has been minimized.

@copumpkin

copumpkin Nov 14, 2017

Member

Thanks! Otherwise it looks great, and thanks for fixing it!

@copumpkin

This comment has been minimized.

Member

copumpkin commented Nov 14, 2017

@grahamc any idea how we ended up with both rebuild-*: 0 and rebuild-*: 1-10?

@NeQuissimus NeQuissimus force-pushed the NeQuissimus:minikube_localkube branch from 0abf0c7 to aac784f Nov 14, 2017

@NeQuissimus NeQuissimus merged commit 905b7a6 into NixOS:master Nov 20, 2017

7 checks passed

grahamcofborg-eval Evaluation checks OK
grahamcofborg-eval-nixos-manual Finished running nix-instantiate ./nixos/release.nix -A manual
grahamcofborg-eval-nixos-options Finished running nix-instantiate ./nixos/release.nix -A options
grahamcofborg-eval-nixpkgs-manual Finished running nix-instantiate ./pkgs/top-level/release.nix -A manual
grahamcofborg-eval-nixpkgs-tarball Finished running nix-instantiate ./pkgs/top-level/release.nix -A tarball
grahamcofborg-eval-nixpkgs-unstable-jobset Finished running nix-instantiate ./pkgs/top-level/release.nix -A unstable
grahamcofborg-eval-package-list Finished running nix-env --file . --query --available --json > /dev/null 2>&1

@NeQuissimus NeQuissimus deleted the NeQuissimus:minikube_localkube branch Nov 20, 2017

@grahamc

This comment has been minimized.

Member

grahamc commented Nov 20, 2017

@copumpkin if master or the branch fails to evaluate it assumed 0.... Then when it was fixed, it was too lazy to remove out of date labels. :) Both of these have been fixed today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment