syncMyMoodle icon indicating copy to clipboard operation
syncMyMoodle copied to clipboard

added dockerization

Open felixevers opened this issue 3 years ago • 6 comments

This pr adds dockeriaztion to syncMyMoodle. Perhaps the maintainer could integrate DockerHub into the CI to provide an image publicly.

felixevers avatar Jun 22 '21 17:06 felixevers

Docker Compose is unnecessary as this just uses a single container. Instead the proper docker flags are sufficient and do not require one to install docker-compose (also makes this more podman friendly)

septatrix avatar Jul 01 '21 16:07 septatrix

Personally I like using docker-compose even for single containers, since you can neatly save your arguments in a .yml file rather than having to remember or saving them to a single line launch.sh. I often set them up myself if they don't come with the project, and a template simplifies that process.

The arguments used for the compose preset could be provided in parallel in the README (and also specific for podman if required), I don't think having both hurts but the README should be updated accordingly.

@septatrix you seem to have a strong opinion. Is there a best practice that fulfills this usecase? Can you explain why it is better or compose is actually bad? From what I hear (not a podman user myself), a simple compose file such as this should even be compatible to podman-compose, if a podman user wants to use that.

HarHarLinks avatar Jul 05 '21 13:07 HarHarLinks

There is nothing inherently wrong with a compose file though I think the most important part of this PR is the Dockerfile. I am fine with keeping it but I think it would be nice to also have the docker one-liner stated in the README.md

septatrix avatar Jul 05 '21 14:07 septatrix

I haven't tested, but does the container support the "secret service" feature? If not, can it be made to?

HarHarLinks avatar Jul 07 '21 23:07 HarHarLinks

I haven't tested, but does the container support the "secret service" feature? If not, can it be made to?

Kinda yey but actually no and no. I assume you mean the secrets mechanism of docker etc. The container itself does not need to do anything special but the script itself does not support them. The mechanism works by simply mounting files in the container which contain the secret. This is probably out of scope for this PR but it might be a good idea to create a separate issue for that.

septatrix avatar Jul 10 '21 13:07 septatrix

I made a separate issue for dockers secret mechanism (#69).

However I realised you are probably referring to the freedesktop keyring service. To my knowledge this would require exposing the dbus and/or some socket to docker which you generally do not want. Considering that docker containers run as root and the keyring is tied to your user probably complicated this further. Also keep in mind that the keyring always grants access to all passwords once unlocked so it is also questionable if you really want that.

(Personally I would simply recommend to encrypt your disk (or the file containing the secret) and use the docker secret mechanism (once supported) which is probably as if not even more secure than using the keyring.)

septatrix avatar Jul 18 '21 22:07 septatrix

Closed. Cause of uneccessary critic. You should look at you code first. This is just an really simple dockerization. This is too much discussion for me!

felixevers avatar Oct 27 '22 12:10 felixevers

Closed. Cause of uneccessary critic.

If you feel like some of the suggestion were unnecessary you should say so. However, you did not participate in any kind of discussion making it impossible to communicate.

You should look at you code first.

I did. A lot actually.

This is just an really simple dockerization.

True. Nonetheless there were some questions, some critique and a few suggestions for improvements. One should engage in this process. Simple does not necessarily mean that there cannot be room for improvements.

This is too much discussion for me!

That is okay, but then you shouldn't expect your PR to get merged.

septatrix avatar Oct 27 '22 15:10 septatrix