search

Romern / syncMyMoodle

Synchronization client for RWTH Moodle
GNU General Public License v3.0
72 stars 18 forks source link

added dockerization #62

Closed felixevers closed 2 years ago

felixevers commented 3 years ago

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

septatrix commented 3 years ago

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)

HarHarLinks commented 3 years ago

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.

septatrix commented 3 years ago

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

HarHarLinks commented 3 years ago

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

septatrix commented 3 years ago

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 commented 3 years ago

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.)

felixevers commented 2 years ago

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!

septatrix commented 2 years ago

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.