-
Notifications
You must be signed in to change notification settings - Fork 308
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
Create ./volumes at cloning #555
base: master
Are you sure you want to change the base?
Conversation
d35c692
to
0304361
Compare
My view is that build-hooks and other IOTstack menu artifacts should adapt to the reality of what docker/docker-compose do by default, never the other way around. In general, I'm not a fan of the menu or anything else futzing about in The acid test should be:
Whatever happens within that re-created volumes folder in terms of ownership and permissions should be the reference model. It should be treated as immutable fact. If a container needs something else within its own area, the container should make the change. That all said, if there's a very very good case where a build-hook is to be preferred over a Dockerfile-based solution, and the build-hook needs more permissions than it has now, it should escalate with For all those reasons, I'm afraid that this PR isn't going to get a thumbs up from me. Not that anybody put me in charge, of course. I'm simply expressing a view based on what happened with 👎 |
I could also add that nextcloud (which, unless I've misunderstood what I was reading on Discord, was the trigger for this PR) works perfectly well under the "acid test" mentioned above. In other words, whatever the nextcloud menu / build-hook is doing is unnecessary. For nextcloud, the only thing that needs to happen in the menu is to set the passwords for nextcloud and its database. No changes are needed in the volumes area. I'm not saying this will necessarily be true for every container where an existing hook futzes about inside volumes (I haven't run every container where IOTstack has a template). But it's definitely true for nextcloud. |
Yes, the menu shouldn't go touching permissions of files mounted into containers. Lets label the different levels:
Only the third one is something that is touched and used by the container. It's mounted as The first two, on the other hand, are just parts of the project structure. What owner or permissions they have make no difference to the service. It's only about convenience.
Change this to: No need to cut off the hand in order to drop everything. There is no container that can control the ~/IOTstack/volumes-folder, only its subfolders may be mounted.
I'll add a fourth point to the ACID test: This PR is created because these steps cause menu to fail this test:
Yes, nextcloud's build.py also has creation of folders that are mounted, and such things should be removed. I may create another PR to cleanup it. But the error in question is not about a mounted Docker volume folder. Unlike wireshark, nextcloud mounts volumes from as subfolders to ./volumes/nextcloud. As such ./volumes/nextcloud is part of the project infrastructure and may be pre-created using a convenient filesystem owner. But using the acid test would create it owned by root. Such ambiguity is bad. The problem is that the build.py does lots of things without which the nextcloud menu-selection wouldn't work. And I feel like that is properly fixable only after #505. |
0304361
to
0f0d599
Compare
Menu usually won't explicitly create the ./volumes -folder, hence Docker will create it as owned by root. This will later cause problems for build hooks trying to prepare directories into a now root-owned folder. E.g. adding nextcloud later will fail with: Error running PreBuildHook on 'nextcloud' [Errno 13] Permission denied: './volumes/nextcloud' Creating volumes as part of the initial git-clone will ensure it's owned by the correct user. For existing users, also add command to fix its owner when the menu is started.
0f0d599
to
3c8c2b7
Compare
Is this change a good idea boils down to: Will changing the owner of The benefits are:
|
DRAFT - Do NOT merge
Letting this ruminate for a bit while trying to understand if this may cause problems down the line.
Make ~/IOTstack/volumes/ explicitly part of the project structure.
It's clearer that
./volumes
exists right from the first checkout, rather than being created either by menu or Docker.Even selecting some menu services and building the stack won't create the ./volumes -folder when. Hence Docker will create it when it creates service volume mountpoints. This results in volumes being owned by root. This will later cause problems for build hooks trying to prepare directories into ./volumes.
E.g. adding nextcloud from the menu when ./volumes is root-owned, will fail with:
Creating volumes as part of the initial git-clone will ensure it's owned by the correct user.