Skip to content
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

Add Dockerfile #392

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add Dockerfile #392

wants to merge 8 commits into from

Conversation

yatima1460
Copy link

Very simple Dockerfile, tried it on my Raspberry Pi 4 and it works perfectly

Good starting point for other people that want to containerize a syncplay server

Copy link
Contributor

@daniel-123 daniel-123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see, a docker file wouldn't hurt. But if it's there it should allow for using full functionality of the server. Especially TLS, but all the other command line switches maybe should be exposed?

I would also like if the dockerfile was as low maintenance as possible - hence I'd prefer no hardcoded versions anywhere.

@@ -0,0 +1,16 @@
FROM ubuntu:18.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason not to just use ubuntu:latest here.

ENV MOTD="Welcome to our Docker Syncplay server!"
ENV PORT=8999
ENV PASSWORD="changethis"
ENV SALT="changethis"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no default salt value. Having a single default implies that all default docker images would always end up with the same salt. Which for the most part defeats entire purpose of having it in first place.


ENV MOTD="Welcome to our Docker Syncplay server!"
ENV PORT=8999
ENV PASSWORD="changethis"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about having a default password. As it stands the docker file will result in server image that cannot be started in non-passworded mode.

Dockerfile Outdated
RUN apt-get install -y make python3 python3-twisted python3-pyside

COPY . /syncplay
RUN cd syncplay && make install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do make install-server instead. Right now the difference is fairly minimal, but it does result in smaller installation.

Dockerfile Outdated
ENV SALT="changethis"

RUN apt update
RUN apt-get install -y make python3 python3-twisted python3-pyside
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best practices recommend consolidating these two RUN statements in a single one, see e.g. here: https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run. Perhaps you could also clean up the package index to make the image smaller.

@albertosottile
Copy link
Member

Thanks for submitting this.

Personally, I would have loved to avoid depending on Ubuntu packages, so to use e.g. python:3.8 as base image and install all dependencies via pip. Unfortunately, this is not possible on all platforms (a notable case is Raspberry PI 3 where cryptography PyPI wheels do not work).

So, I think this proposal is a good first iteration. We can always improve it as soon as the ecosystem supports it.

@FallenWarrior2k
Copy link

Hello, I just noticed this PR, and since I recently upstreamed some fairly comprehensive changes to the currently most popular Syncplayer Server image on Docker Hub (NinetailLabs/syncplay-server#5), I was wondering if (some of) those could be incorporated here.

Theoretically, you could even ask the maintainer of that if you could officially "adopt" some of the files, seeing as they're under Unlicense.

@yatima1460
Copy link
Author

Yeah this is still a bit of a mess and using a huge FROM ubuntu is ugly

But hey, it works!

Next steps are probably to make it slimmer and cleaner with few layers

@yatima1460
Copy link
Author

yatima1460 commented May 13, 2021

Meanwhile I added a HEALTHCHECK by checking if the port is open, maybe there is a better alternative but I don't know Syncplay internals, it could be nice if Syncplay would return 200 OK on 127.0.0.1:8999 for example and then use curl in HEALTHCHECK, instead I get a ERR_INVALID_HTTP_RESPONSE

@daniel-123
Copy link
Contributor

When it comes to responses of Syncplay server - it's not HTTP server, so it doesn't really understand HTTP. If it receives anything except its native json formatted protocol it will throw it back at the requester as part of error message. Relevant code is right here.

That said, headerless response it gives back technically is within spec for HTTP 0.9, so you can force curl to check if it's alive:
curl localhost:8999 --http0.9 and you should get {"Error": {"message": "Not a json encoded string GET / HTTP/1.1"}. While this is not very elegant, at very least lack of timeout here does mostly prove that the server is still running.

When it comes to incorporating what @FallenWarrior2k did, at very least switching to python:latest or python:3.9 base image sounds like a good idea.

@FallenWarrior2k
Copy link

I'm generally not a fan of using latest for base images, as it makes the resulting images non-reproducible, i.e. an image pulled from Docker Hub might work, while the same image built locally could behave completely differently or not work at all. Most critically, as far as I know, tags don't get automatically pulled if present locally. While this generally isn't much of a problem with fixed versions, building from a latest base image might use a completely outdated version, depending on when the user last used that updated that image.

Moving away from the local build aspect, it also introduces the risk of spurious failure of automated builds. For example, in Alpine 3.12, the man package was renamed to mandoc (more complex, but that's the gist), which broke many things that were just using FROM alpine or FROM alpine:latest instead of specific versions.

@daniel-123
Copy link
Contributor

There are several very good reasons not to pin base image version here:

  • We specifically want failures to happen at very least in our CI processes to have heads up that something is wrong early.
  • The image generated will always be on top with regards to security updates.
  • Both ubuntu and python images labelled as "latest" I mentioned are very much stable (based on either Ubuntu LTS or Debian Stable respectively) - they do not randomly introduce breaking changes and they do provide regular security updates. I didn't say anything about Alpine earlier because I don't know anything about its commitments to stability and release schedules.
  • Pinning a version means it will have to be updated with each minor upstream release which is going to be yet another thing requiring constant attention.
  • Avoiding building with stale base image is responsibility of the end user and their toolchain. Using docker-compose or basically any other tool for that solves this problem.

@albertosottile albertosottile added enhancement stale Marked for deletion labels Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement stale Marked for deletion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants