Flexible Local Folder for Volumes #1202
Conversation
|
👍 thanks for creating this PR - I'll check it out tomorrow, but at first glance it seems that you found a good way to check for shared folders. Can you confirm that this also compiles properly on Windows? |
|
Yeah, I'm using Windows as dev environment so I only could check it works My current version is W10 Build 10586. I'm sure we can optimize the code but I think it's a good first approach to Anything you can reach me here :)
|
|
|
||
| if (!directory || directory.indexOf(util.home()) === -1) { | ||
| var founded = false; | ||
| if(directory){ |
There was a problem hiding this comment.
Can you fix the indentation and check that spacing around keywords 'if' are respected?
|
Hi @FrenchBen, Sorry for the things you point out. You are absolutely right. My bad! I think I push all the changes you requested. Please, take a look if now is OK for you and sorry again. |
| @@ -82,6 +82,9 @@ export default { | |||
| machineVersion | |||
| }); | |||
|
|
|||
| virtualBox.getShareDir(machine.name()); | |||
There was a problem hiding this comment.
If a user adds a shared directory, it will only be seen until the next kitematic restart - Is this the correct approach?
There was a problem hiding this comment.
What you are comment is totally right. I think that the operation of add a new folder to the machine is not a "normal" activity when you are working, it is more usual an activity you do so few times when you are configuring your enviroment. So, I think recovering once when the program starts is enough, but if you think it's better another approach we can do this call when anyone is trying to change the volume..
I'm open to all your considerations, you are the master here :)
There was a problem hiding this comment.
haha no such thing as the master, just an observation. I think you're right, the user would add more folders and restart or start kitematic after making the changes.
When in doubt restart, right?
|
Doing some QA on the code you currently have I'm having a couple of issues while using the hello-world-nginx
Also mounting a volume on my Desktop (within the user dir), removing it and closing kitematic and opening it again, I get the following within your repo:
I confirmed this doesn't happen on Master. |
|
Hi @FrenchBen , sorry to hear that, I'm going to try to reproduce it here, and see which is the root cause. I'll get back to you. |
|
Hi @FrenchBen , I think I fix the bug you pointed out. The problem was I removed the "No Folder" condition and when the source was null, it crashes! Sorry! My bad! please, check it now and if you have any problem, I could fix it right away! |
|
Any more feedback on this PR? Thx! |
|
Wont' be able to look at it till monday - meanwhile, can you squash your commits into logical commits, instead of the multiple "fix this" "removed that"? |
|
Of course I will do. Thanks for everything -- Sent from my mobile phone. Please excuse any typo.
|
|
Excited for this feature! Anyone have an ETA? |
|
Hi @FrenchBen . I have squashed the commits here as well to fix the same thing that you reported in the other PR . I hope with this change everying is OK. Regarding to the @kotowick we are doing some QA to see that there is no bugs in the PR before any kind of merge. Regarding to the merge I have no clue about that but I'd like to be soon!!! 👍 |
|
Thanks @alexandrev, looking forward to this feature 👍 |
|
@alexandrev @FrenchBen hoping to have this in the next release? |
|
I mounted a folder from the tmp dir in VirtualBox and made it 'automount' + 'permanent' I then opened Kitematic based on your PR, created a container based on the The problem was that the 'index.html' file that normally gets created/copied into the volume mounted, didn't get copied. Are you able to replicate this issue? If so, do you know why this is happening? |
|
I'm going to try to reproduce it, and I'll come back to you with some answers :) |
|
Hi @FrenchBen, I'm going to describe my steps to reproduce the issue you are facing. Please, let me know if I'm doing something wrong to do it. First, I add a new local folder inside the Docker VM machine, as you can see here:
Then, I specify the mapping in the Volume section as you can see here:
I start the container as you can see here:
And I can found the index.html file inside the DockerVM file:
Is everything OK? Or am I missing something? Thanks! |
|
Weird on my machine, going to |
|
I don 't do anything special to get it mounted. I even not mount the -- Sent from my mobile phone. Please excuse any typo.
|
|
My question was how you add the folder to VirtualBox? |
|
Here you are, my virtual box shared folder configuration:
|
|
Thanks to you because without your script, this could never be posible :) |
|
@FrenchBen maybe you could take a look at this and give us your comments :) |
|
Any more QA have been done? @JeffDM , what do you think about the feature and the code? Any comment? |
|
I've been using this branch on a daily basis for past few days and encountered no problems. Would love to see this in production. |
|
Ok!! Thank you @extra-0. @JeffDM and @FrenchBen does it look good for you? |
|
@alexandrev compiled and works well for me - haven't ran into any issues. Thanks for doing this! @FrenchBen let's get this merged :) |
|
@FrenchBen Any particular reason for this not to be merged? Anything needs to be changed? |
|
@extra-0 I'd like for the solution to be within boot2docker more than a hack from our side to get it working - Having This only solves the kitematic side of things, when this could benefit all other users (cli users) - If it's done accordingly within the boot2docker setup, then everyone can benefit from it. |
|
@FrenchBen I believe we didn't use any hacks. On the contrary, we exploited functionality that was already there in boot2docker. Isn't Kitematic all about automation? We just automated a process of mounting volumes as user changes settings or starts new containers. It's extremely flexible and by my experience brings Kitematic users to the next level without binding them to use home folders only. Please, check it out! |
|
@FrenchBen, I don't see the problem of using sudo inside a VM. It's like using it inside a sandbox. Which is the worst thing could happen? the problem will be using it inside the host machine. @extra-0 Do we have an issue open to boot2docker to handle this issue? Because I think we didn't open any issue there... |
|
@alexandrev I just browsed through your code again and found no mentions of sudo. Am I wrong? Also, I suppose boot2docker doesn't have an issue with functionality we are using, but rather an issue with remounts after reboot. We could create a ticket in their repository, however mounts that are used by your version of Kitematic handle this in another way by mounting folders when they are needed. |
|
We have a sudo when we do the mkdir and mount inside the boot2docker Yeah, I'm aware our action is different but if @FrenchBen as core maintener Thx man! On Tue, Mar 1, 2016 at 10:09 AM extra-0 notifications@github.com wrote:
|
|
@FrenchBen @alexandrev There were several tickets and even a pull-request at boot2docker to fix this, however it has not yet been accepted and this issue does not get much attention in boot2docker. |
|
Sudo is targeted to run in a 'sandbox' but say something odd happens and the Having the fix in boot2docker would make this work across cli and kitematic. Having the desktop app include more functionality than is available via the normal CLI seems a bit conter-intuitive. If you don't mind pinging the devs in your last issue first, it would be appreciated: If we don't see anything within the next 2 weeks then we can all vote to merge it and provide more power to Kitematic user :D Agreed? |
|
I agree @FrenchBen. But I want to talk about the sudo "problem". The sudo a) Windows platform: Nothing happens. Thanks!! On Wed, Mar 2, 2016 at 1:11 AM French Ben notifications@github.com wrote:
|
|
Hi, I want to review my open #PR and see if there is some need to change anything to merge them or close them if they are not needed. I don't know if this PR makes sense right know after the big change of Docker for Windows / Mac and the change from VirtualBox to Hyper-V and the other option on Mac. @FrenchBen do you need something from my side or do we close this PR? Thanks! |
|
I think this can be closed - I submitted a PR that checks for an existing Docker for Mac/Windows install and allow other folders to be mounted based on that. Are you ok with me closing it? |
|
@FrenchBen I think we have to focus on the newer versions Docker for Windows and Docker for Mac. So, if it is solved there. I'm ok with you closing it. But I think I need to push the transition to Kitematic for the new versions. Because right now it is very difficult to collaborate on things when the master repo seems to be more related to the 'old Virtualbox-based Kitematic' The main idea here, is that all of us have to work on the same direction to do greater things :) |
|
@alexandrev absolutely - I merged a couple of PR related to the changes for folders to be reachable outside of the |
|
I am on macOS Sierra and it is not clear how to add new volumes as a result of this feature, particularly when virtualbox is not being used. 376 appears to address the issue of adding new volumes to the dockerfile. |
|
@jhgorse the above feature only applies to native users. If you're on Docker for Windows, you simply need to use the settings window to add the volumes that should be shared. See: |
|
@FrenchBen I have what I believe to be the native docker-beta for macOS Sierra, which also has the shared directories (default /Users:/Volumes:/tmp:/private). These do not appear in the Kitematic->Settings->Volumes settings. |
|
@jhgorse The 'add a folder' screenshot is from Virtualbox - Kitematic will not show any folder if the image that you're using doesn't have a volume directive. Adding a folder when one isn't declared in the image, has yet to be implemented with the latest changes. This was started in #722 which was closed in favor of this one, which solved only part of the issue. |
|
Still getting error.
I deleted the machine in virtual box. @FrenchBen Note: I am wanting to use VirtualBox on Windows 10 as I can't seem to use VBox with the native windows emulation at the same time and I have many VBox vm's. And the native windows way didn't work well with my computer/network card for some buggy reason it was unbearably laggy. (Broadcom) |
This change allows to define Volumes where the local folder is not limited to the Users directory but to any folder mounted in the VM.
It recovers from the VBoxManage the mounted folders and it checks the final folder agains this values, and if it matches it allows to specify the path.
This code is related to Issue #1192 opened as a question.
Please, if you have any question, only let me know.
Signed-off-by: Alexandre Vazquez alexandre.vazquez@gmail.com