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
Use independent Docker image to build assets #7992
Conversation
Currently to build assets we need to install nodejs locally, install all the node packages locally and call `npm ci && bower update && npm run build`. This has some downsides due that all of this commands are not ran inside the docker container: - we need to have all the Python requirements installed locally - `manage.py collectstatic` is run in the host machine - all the static files are not copied to the S3 storage backend - we don't know what version of `nodejs` is the valid one To avoid all of these issues, this commit creates a new invoke task: inv docker.buildassets which pins the version of node used, runs all the node commands in an isolated docker container and finally runs `manage.py collectstatic --noinput` inside the `web` container, which will copy all the new assets to the S3 storage backend automatically for us. (requires a PR in readthedocs/common repository)
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.
This is a solid change. 👍
@@ -0,0 +1,8 @@ | |||
version: '3' |
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.
Why is this a different file? I find it nicer having everything in 1 place, so we don't have to check 6 different files for docker config.
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.
The nodejs container is not needed to run the development instance (and vice-versa). We use the same pattern here than the one for search/ext-them, which are optional as well.
|
||
services: | ||
assets: | ||
image: node:8.16 |
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.
This is a really old version, we were generating the assets with 10.17 locally
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.
8.16 is the one that I've been using all this time. I know people are using different versions, but I don't know which one is using each of us.
I'm happy to update to any other one, but I haven't test them and I don't want to jump into that if it does not add anything to us. It use to break just randomly 😄
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.
You can see our docs mention 10.17 should be used https://github.com/readthedocs/readthedocs.org/pull/7992/files#diff-2f342d78afce7705517e17179ba910d04ba83b37d0d3d4bd0bbacfdec256f6c0L58
Currently to build assets we need to install nodejs locally, install all the
node packages locally and call
npm ci && bower update && npm run build
. Thishas some downsides due that all of this commands are not ran inside the docker
container:
manage.py collectstatic
is run in the host machinenodejs
is the valid oneTo avoid all of these issues, this commit creates a new invoke task:
inv docker.buildassets
which pins the version of node used, runs all the node commands in an isolated
docker container and finally runs
manage.py collectstatic --noinput
inside theweb
container, which will copy all the new assets to the S3 storage backendautomatically for us.
(requires a PR in readthedocs/common repository: readthedocs/common#85)