adlogix / docker-machine-nfs

Activates NFS on docker-machine
MIT License
794 stars 104 forks source link

Update compatibility to docker-machine 0.5 and add support for parallels driver #19

Closed blueimp closed 8 years ago

blueimp commented 8 years ago

On docker-machine version 0.5.0, the machine state was not properly parsed and resulted in an error. This has been fixed by making use of AWK's field print function.

The NFS mount verification also failed as the mount was not yet available directly after restarting the virtualbox machine. This has been fixed by doing the mount check in a loop with a sleep timer.

As a new feature, support for enabling NFS with the parallels driver has been added.

Please note: The atom editor also fixed up some superfluous whitespace. You can hide the whitespace changes by adding ?w=1 to the URL.

michaeljs1990 commented 8 years ago

Awesome, just fixed the issue for me. However the original script does actually work it just reports that it doesn't. Not sure if this script will be backwards compatible.

krasilich commented 8 years ago

Seems it doesn't check status correctly if I have more than one machine.

$ docker-machine ls
NAME      ACTIVE   DRIVER       STATE     URL                         SWARM
dev       -        virtualbox   Running   tcp://192.168.99.100:2376
dev2      -        virtualbox   Stopped
dev-prl   -        parallels    Stopped
$ docker-machine-nfs dev
[INFO] machine presence ...             OK
[INFO] machine running ...          FAIL

The machine 'dev' is not running but 'Running
Stopped'!
blueimp commented 8 years ago

Thanks @krasilich, I've updated the script to fix this issue.

krasilich commented 8 years ago

Ok, seems like it works for me now, waiting for PR merge, thank you @blueimp

blueimp commented 8 years ago

You're welcome, thanks for QA!

tonivdv commented 8 years ago

Hey @blueimp ,

Thanks for contributing. I just checked with docker machine 0.4.1 and it doesn't work. That being said, not sure if supporting older machine versions makes sense ...

If we merge this we should at least update the readme to notify users that the script only works as of 0.5.

If anyone feels this script still needs to work with <0.5 then I could always branch ...

Last but not least, when you are finished with the readme, can you squash all the commits.

Thanks again for the effort.

Cheers

michaeljs1990 commented 8 years ago

@tonivdv I personally wouldn't support older version of docker-machine until it gets past 1.0 however i think making a tag that corresponds with the docker-machine version would be appreciated by some people. The readme could say master works with the latest version of docker machine for older versions people checkout the appropriate tag.

tonivdv commented 8 years ago

@michaeljs1990 Yup that makes sense :) :+1:

blueimp commented 8 years ago

@tonivdv: Done.

Changes in this pull request:

tonivdv commented 8 years ago

Thanks @blueimp . I will test this tomorrow and merge if I don't encounter any issues.

Just for to future, I'd prefer to not put many different things in 1 PR. For me fix compatibility for 0.5 and adding support of parallels are 2 different things, and if the latter would cause issues after merge, it would be "less easier" to rollback.

But I do appreciate all the effort you put into this ;)

Cheers

blueimp commented 8 years ago

Thanks and understood, @tonivdv!

tonivdv commented 8 years ago

Thanks again @blueimp

blueimp commented 8 years ago

You're welcome and thanks for merging!