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

Removed systemd dependency from minikube, updated none driver to refl… #1592

Merged

Conversation

aaron-prindle
Copy link
Contributor

…ect this

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 15, 2017
@codecov-io
Copy link

codecov-io commented Jun 15, 2017

Codecov Report

Merging #1592 into master will decrease coverage by 0.02%.
The diff coverage is 38.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
- Coverage   38.66%   38.63%   -0.03%     
==========================================
  Files          51       51              
  Lines        2607     2658      +51     
==========================================
+ Hits         1008     1027      +19     
- Misses       1421     1450      +29     
- Partials      178      181       +3
Impacted Files Coverage Δ
pkg/minikube/kubeconfig/config.go 50.46% <0%> (-0.97%) ⬇️
cmd/minikube/cmd/start.go 16.47% <0%> (-0.59%) ⬇️
cmd/util/util.go 33.33% <0%> (-5.13%) ⬇️
pkg/minikube/cluster/cluster.go 44.37% <50%> (+0.33%) ⬆️
pkg/minikube/cluster/commands.go 62.06% <82.6%> (+4.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a3e204...6639b2d. Read the comment docs.

@@ -76,6 +92,26 @@ func GetStartCommand(kubernetesConfig KubernetesConfig) (string, error) {
return buf.String(), nil
}

func GetStartCommandNone(kubernetesConfig KubernetesConfig, localkubeStartCmd string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename this to GetStartCommandNoSystemd or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -96,10 +91,19 @@ func (d *Driver) GetURL() (string, error) {
}

func (d *Driver) GetState() (state.State, error) {
command := `sudo systemctl is-active localkube 2>&1 1>/dev/null && echo "Running" || echo "Stopped"`
var localkubeStatusCommand = fmt.Sprintf("if [[ `systemctl` =~ -\\.mount ]] &>/dev/null; "+`then
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be out of scope for this fix, but why do we have this variable in two places?

@aaron-prindle aaron-prindle force-pushed the remove-systemd-requirement branch 4 times, most recently from ef3c5d5 to 12d1604 Compare June 20, 2017 16:43
@@ -246,3 +247,34 @@ func KillMountProcess() error {
}
return mountProc.Kill()
}

func MaybeChownDirRecursiveToMinikubeUser(dir string) error {
if os.Getenv("CHANGE_MINIKUBE_NONE_USER") != "" && os.Getenv("SUDO_USER") != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this first os.Getenv check? It looks like you only call this if CHANGE_MINIKUBE_NONE_USER is true.

Copy link
Contributor Author

@aaron-prindle aaron-prindle Jun 20, 2017

Choose a reason for hiding this comment

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

@@ -577,15 +574,20 @@ func EnsureMinikubeRunningOrExit(api libmachine.API, exitStatus int) {
// RunCommand executes commands for both the local and driver implementations
func RunCommand(h *host.Host, command string, sudo bool) (string, error) {
if h.Driver.DriverName() == "none" {
cmd := exec.Command("/bin/sh", "-c", command)
cmd := exec.Command("/bin/bash", "-c", command)
Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked and we do have bash. Nice.

@r2d4 r2d4 mentioned this pull request Jun 20, 2017
@r2d4
Copy link
Contributor

r2d4 commented Jun 20, 2017

we can probably remove vm_systemd.go from the integration tests

@r2d4
Copy link
Contributor

r2d4 commented Jun 20, 2017

we can probably remove vm_systemd.go from the integration tests

ref #1616

@aaron-prindle aaron-prindle merged commit 5c2b93c into kubernetes:master Jun 20, 2017
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants