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

Dockerfile and docker support #953

Draft
wants to merge 6 commits into
base: 3.0
Choose a base branch
from
Draft

Conversation

davidsiaw
Copy link

@davidsiaw davidsiaw commented Sep 19, 2022

I have recently needed to update my branch to the latest changes of the cytube branch and redid the Docker support for my own purposes.

I realize this PR is not immediately mergeable due to several issues:

  1. cyzon may not want the Dockerfile to be in this repo
  2. there are some invasive changes to the package json
  3. it is not clear how to use the Dockerfile correctly (the README additions are very basic and assume a lot of things). I am open to improving the documentation for these purposes.

The changes are a lot simpler compared to when I last proposed docker support (5 years ago) due to advances in both Docker and the MySQL images that are now available, and improvements to cytube that no longer required the invasive changes I once needed to do to get it to work.

Unfortunately I have not thought deeply about how we can incorporate these changes as part of cytube-docker, which has been maintained by someone else for a while now. It is my opinion that these changes are much simpler being in the repository itself.

I am putting this PR in the hopes that perhaps someone will find it useful and perhaps someone will find this the key to providing more meaningful docker support in the future, and also to serve as a point of contact for anyone who wishes to ask me questions about how to set up cytube on Docker.

There are several things that can/need to be improved

  1. Moving the startup to a non-root user internal to the container
  2. Making the environment variables used to configure the server better
  3. Automatically selecting cloudflare IPs instead of hardcoding them in the config template as it is right now
  4. Automatically build a docker container and send it up to dockerhub. (this no longer requires giving perms to dockerhub for anything anymore)

It is my hope that we can find a path forward towards some level of rudimentary Docker support that does not need maintenance, as I did not have to do much to make this work and it is not very complicated to get it working.

In any case if cyzon does not wish for an open ended PR to be here he is free to close it.

@calzoneman
Copy link
Owner

If you wanted to try to get this into a mergeable state, I'm open to working with you on that review.

Regarding the cytube-docker repository, I'm not sure it's really maintained, the last commit is 3 years ago and I'm not sure anyone is using it. I think the concept is similar, with the main difference being that that repo provides a Docker compose file to model the application + MariaDB stack, which probably makes it a bit easier for users to get started.

@davidsiaw
Copy link
Author

Thanks. I am cleaning up the sources now to make them easier to inspect, and the next steps I will take is to remove some of the changes that turn out to no longer be necessary such as my old modifications to the cytube filters.

My goal here is to introduce a Dockerfile that will produce a docker image that anyone who uses docker-based solutions can readily pull in, and that the Dockerfile will require very little maintenance, and that most relevant configuration for docker is exposed via environment variables.

I would like to hear any concerns you may have.

@davidsiaw
Copy link
Author

Alright, so this is more or less all that you need to launch cytube on docker. This and knowledge of how to start up a mysql container and expose your ports (basic docker server knowledge).

All that is needed now is possibly an example docker-compose.yml or some documentation on how to set it up for people who just want to paste some stuff onto their console.

My goal is to make it so one can create a container image

docker build -t cytube .

And then run it

docker run -d --name db --network cytube_network ... mariadb
docker run -d --network cytube_network -p 8080:8080 cytube

And have a cytube.

I can go on to write some of that documentation when I catch a breather.

Copy link
Owner

@calzoneman calzoneman left a comment

Choose a reason for hiding this comment

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

Sorry it took me a while to get back to you.

ENV MYSQL_PORT 3306
ENV MYSQL_DATABASE cytube
ENV MYSQL_USER cytube
ENV MYSQL_PASSWORD nico_best_girl
Copy link
Owner

Choose a reason for hiding this comment

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

For things like passwords (and the root URL, cookie secret, etc.) where we expect the user to always need to customize it, I think it would be preferable to leave it unset by default so that users are forced to fill in the appropriate value (usually missing values present a more obvious error message than silently trying to use some default value).

Copy link
Author

Choose a reason for hiding this comment

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

These are placeholder values, since docker does not allow the ENV command to have empty values. The user should set these values using the -e flag when starting the container. Building the container should be a one-time thing that is done by a CI task

ENV ROOT_DOMAIN localhost:8080
ENV HTTPS_ENABLED false
ENV COOKIE_SECRET aaa
ENV IMMEDIATE_PROXY 172.16.0.0/12
Copy link
Owner

Choose a reason for hiding this comment

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

Is this documented Docker behavior or an implementation detail? I also assume this would only do anything if you also had a reverse proxy container running (e.g. nginx) in front of cytube?

Copy link
Author

Choose a reason for hiding this comment

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

This is a docker implementation detail. 172.16.0.0/12 is a known value, though users can change it, its usually left to be that. https://docs.docker.com/network/network-tutorial-standalone/

README.md Outdated
--network sync sync
```

Feedback
Copy link
Owner

Choose a reason for hiding this comment

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

What's this blank section for?

Copy link
Author

Choose a reason for hiding this comment

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

Its just a space between the contents and the feedback section.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I meant the Feedback section itself is blank (there is a heading but nothing under it).

@@ -37,6 +37,38 @@ General help with the software and the website is also available on the IRC
channel at [irc.esper.net#cytube](http://webchat.esper.net/?channels=cytube)
during US daytime hours.

Docker
Copy link
Owner

Choose a reason for hiding this comment

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

I would maybe put the docker instructions in a separate file (e.g. docs/docker.md) and then add a link to that from the top of https://github.com/calzoneman/sync/wiki/CyTube-3.0-Installation-Guide in case people want to use docker instead of following the usual installation instructions.

Comment on lines +3 to +6
apk update
apk add build-base python3 git npm mysql mysql-client curl gettext ffmpeg
npm install npm@latest -g
npm install
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be preferable to just run these as separate RUN commands in the Dockerfile so that you can take advantage of Docker's caching and only re-run what is necessary (e.g., if you modify the source files, then you probably need to re-run the npm commands, but it's not necessary to reinstall all the apk dependencies).

Copy link
Author

Choose a reason for hiding this comment

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

I placed them in a script file as a group of commands that don't have to be changed all the time, and are just peculiar to cytube.

Having them as separate RUN commands is only useful if you are constantly re-building the image, like when you changed cytube to the point where we need to install something in a very specific way, and hence we need to re-run the Dockerfile multiple times.

(The image should be built by a Github action)

Copy link
Author

@davidsiaw davidsiaw Oct 23, 2022

Choose a reason for hiding this comment

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

just peculiar to cytube.

just required by cytube, not really peculiar or anything. They shouldn't have to change much if ever, unless we require a new dependency or something.

run.sh Outdated Show resolved Hide resolved
@willjr880
Copy link

Hello, I didn't realise something was in the works already. I have created a docker container (with the expectation that you run it behind a reverse proxy.) So far it appears to be working as expected.
https://github.com/willjr880/cytube_docker

Feel free to use to your liking.
Wil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants