-
Notifications
You must be signed in to change notification settings - Fork 5
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
Added support for hosting website via nginx docker #153
Added support for hosting website via nginx docker #153
Conversation
Apologies, I did not mean to close / delete my docker branch. |
You are valid. I like this PR. I sat on it for a while because I'm not super familiar with docker (then I went on 🏝vacation for a week). |
Dockerfile
Outdated
WORKDIR /app | ||
COPY package.json ./package.json | ||
COPY yarn.lock ./yarn.lock | ||
RUN yarn install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RUN yarn install | |
# git is needed to install `jinter` from github | |
RUN apk add git | |
RUN if [ ! -d 'node_modules' ]; then yarn ci; fi |
I got the error: #0 12.23 error Couldn't find the binary git
when I tested this merged into the latest commit on development
.
This is because, since this PR was made, I added the development version of Jinter
as a dependency (it fixes a bug that was preventing building with the latest version of YouTube.js), so I believe this will also need to have git
installed as a dep for the time being.
Additionally, I like the idea of being able to pass in the existing packed directory instead of rebuilding (because that would make it easier to throw this into the existing CI)
Dockerfile
Outdated
WORKDIR /app | ||
COPY . . | ||
COPY --from=dep /app/node_modules ./node_modules | ||
RUN yarn pack:web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RUN yarn pack:web | |
COPY --from=dep /app/dis[t]/web ./dist/web | |
# don't rebuild if you don't have to | |
RUN if [ ! -d 'dist/web' ]; then yarn pack:web; fi |
This goes along with my comment about using an existing directory.
Sorry if that was a little stilted or out of order, but I basically just modified what you have locally until I got something I could easily run in the FROM node:18-alpine AS dep
WORKDIR /app
COPY package.json ./package.json
COPY yarn.lock ./yarn.lock
# copy `dist` if it exists already
COPY dis[t]/web ./dist/
COPY node_module[s] ./node_modules
# git is needed for jinter
RUN apk add git
# don't rebuild if you don't have to
RUN if [ ! -d 'node_modules' ]; then yarn ci; fi
FROM node:18-alpine AS build
WORKDIR /app
COPY . .
COPY --from=dep /app/dis[t]/web ./dist/web
COPY --from=dep /app/node_module[s] ./node_modules
# don't rebuild if you don't have to
RUN if [ ! -d 'dist/web' ]; then yarn pack:web; fi
# Removing node_modules and src at build stage to reduce image size.
RUN rm -r node_modules src
FROM nginx:latest as app
COPY --from=build /app/dist/web /usr/share/nginx/html |
No worries and thanks for looking at my commit! Your edit looks good! I did not think about skipping install or build if node_modules and/or dist already exist as there might be conflict between different OS/kernel and architecture, but since Node is multi-arch, I do not think it matters at all? I believe we also do not need the following line below.
I have recently learned that in multi-staged builds (build files with multiple FROM statements / bases), all previous stages except the file stage is removed from the final image, so in this case, only the app stage will be bundled in the image. Dep and build stage will be removed automatically. Link for more info on multi-staged build only bundling the last stage.
|
I'll make the changes and commit to my branch! Besides that, there were two concerns that made me think my PR might not completely resolve issue #129. First, I want to create persistent storage for the docker container by allowing users to either bind directories (mounting a host directory in container) or create docker volumes for the config directory (folder with profiles.db and etc. files) in the container. This makes the container and data portable and easy to manage for backup and on the off change the container is corrupted or accidentally deleted. However, I am not sure where all the config files are located in. According to FreeTube's documentation, it is located in "~/.config/FreeTube", but since this is a docker container, there is only the root user and I do not see a .config at root directory. However, data seems to persist even when I shutdown the container and start it again, so it is definitely saved somewhere in disk and not in RAM. If you have an idea at where the config is located in that would be great 😁. Second, might be a good idea to add in a login/password as some of us have other people (like family or friends) on the local network, and prefer to not let them snoop in on what we are watching. This could be done by creating a separate login page with either a GUI or an alert function that asks for username and password specified when the docker container is created (we could pass the container a USERNAME and PASSWD environment variable). |
The changes are working on my end too.
I have modified the Dockerfile to your suggestion! I also kind of like the ability to copy the node_modules and dist from host if available. It really helps to speed up the dep and build stages, as "yarn ci" and "yarn pack:web" can take a while to run. Running these two instructions on the host is a lot faster! There is a way to speed up the two instructions, but it is a bit complicated to set it up. |
For some reason, data like subscription and favorites persist on my machine even when I delete and restart the container... I did not add bind mount or volume... Where does FreeTubeCordova put their config files in? I tried to use find and grep command inside the container and in host, but was not able to find the files. |
Could it be saved as cookies (i dont know too much about the docker setup) |
My apologies, I am not too familiar with FreeTube, and I was a bit confused. I just checked my subscriptions in Firefox and Safari and they are different. It makes sense to me now. There was probably a low chance of a website having the permission to access a config folder on a computer. It made more sense for the config to be stored in the browser's cache.. Thanks! |
Co-authored-by: Emma <[email protected]>
Great, changes committed! |
…ill fail if dist/web does not exist
…dist/web from dep, and specified that building image on Apple Silicon may be slow
Apologies. For some reason, some of the changes we discussed before were not added to the pull request. I have reopened the request with some addition changes besides the one we have discussed. One problem is that conditional copy instruction does not exist in Docker so that means it is not possible to copy ./dist/web if it already exist on host. |
I actually accidentally merged in like half of this a while back, and then, I took a break for a while, so I need to go back and clean some stuff up, but then, this should be getting fully merged. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Looking this back over, I noticed you removed the glob pattern, and you left a comment saying that "There are no conditional copy instructions in Docker", but as far as I understood, glob patterns could be used to achieve a copy that doesn't fail if the file doesn't exist to copy. I guess I am just confused because they appeared to work the way I expected them to. If there is some reason that the conditional copy shouldn't be done or wouldn't work right that I am not understanding, could you explain more? It would just make the CI faster if it doesn't have to redo work. |
Ah sorry, that is completely my fault. I was testing the Dockerfile in Podman (an open source alternative to Docker), and it does not support conditional instructions. Conditionals should work normally in Docker.
|
…nd run statements
… also resolved merge conflict in README.md
Conflicts have been resolved. A maintainer will review the pull request shortly. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Co-authored-by: Emma <[email protected]>
Sorry, that this took absolute ages. |
Added support for hosting website via nginx docker
Pull Request Type
Related issue
#129
Description
Provide support to dockerize FreeTubeCordova.
Added Dockerfile, dockerignore, and modified README.md to add instructions on how to create and run images locally and by Docker Hub. FreeTubeCordova should replace the image, "owentruong/freetubecordova", in the PR's readme with its own published image if available.
Screenshots
Testing
Subscription and video works when tested locally in Chrome, Firefox and Safari.
Desktop