machine-drivers / docker-machine-driver-xhyve

docker-machine/minikube/minishift driver plugin for xhyve/hyperkit (native macOS hypervisor.framework)
https://godoc.org/github.com/machine-drivers/docker-machine-driver-xhyve
BSD 3-Clause "New" or "Revised" License
888 stars 74 forks source link

Fix #179 add process name check to getState #180

Closed gbraad closed 7 years ago

gbraad commented 7 years ago

This fixes #179 by checking the executable name of the process retrieved.

zchee commented 7 years ago

@gbraad Thanks pull request. Now I test that fix. Please wait a while.

praveenkumar commented 7 years ago

@gbraad Thanks, will also test this out and provide feedback.

zchee commented 7 years ago

/cc @dlorenc @r2d4

zchee commented 7 years ago

@gbraad Also, could you add static vendoring github.com/mitchellh/go-ps? We use gvt(for now. Maybe I will switch to dep), so:

go get -u -v -x github.com/FiloSottile/gvt
gvt fetch github.com/mitchellh/go-ps
git add vendor/manifest vendor/github.com/mitchellh
zchee commented 7 years ago

@gbraad And CI are failed. but nevermind. it's caused by OCaml third-party install.

praveenkumar commented 7 years ago

We use gvt(for now. Maybe I will switch to dep)

@zchee what if we can switch to glide. we are using it for minishift quite a long time and works as expected. I can help you with migration to glide.

zchee commented 7 years ago

@praveenkumar Hm, but glide author?(sdboyer) now maintain golang/dep. And dep command already have glide importer. But yeah, I think dep is experimental, glide is production ready. If you really like glide, It’s up to you. switch to glide pull request is welcome.

praveenkumar commented 7 years ago

Tested out this patch and it does work as described only need to put https://github.com/zchee/docker-machine-driver-xhyve/pull/180#issuecomment-311587314 so make build doesn't fail.

gbraad commented 7 years ago

Thanks @zchee, will do this ASAP

zchee commented 7 years ago

@gbraad Thanks. I'll release the new version after check and merge this pull request. FYI, I tested minikube side, it seems works successfully.

gbraad commented 7 years ago

so basically you are checking in a fixed version of the library as part of the repositry, under vendor

gbraad commented 7 years ago

rebased and tested with minikube and minishift. all OK.

gbraad commented 7 years ago

inverted if statement to return errors first.

zchee commented 7 years ago

@gbraad Thanks! I'll check and merge it. No reply from Google side, but seems to no problem this change.

gbraad commented 7 years ago

they are both (@dlorenc, @r2d4) in a far away timezone. it is likely they will see it in about a few hours. But I always test against minikube as we are trying to align with them more. But n their case it is not the default hypervisor.

zchee commented 7 years ago

@gbraad I see. Yes, I also check minikube some commands. So already double checked. I'll merge it. Thanks for contribution.

gbraad commented 7 years ago

maybe have some other changes over time, as the PID code should also check for dead/stale PID files. But this is a first improvement!

dlorenc commented 7 years ago

Looks great to me, thanks!

r2d4 commented 7 years ago

LGTM 👍