For proper build caching, we want to first handle the node_modules, and then later copy in the actual files.
Depends on D3712
Details
Details
- Reviewers
tomek • jim - Commits
- rCOMMca61a68e4d26: yarn cleaninstall from Docker image
Make sure I can build the Node.js keyserver from Docker
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Making @jimpo a blocking reviewer to make sure I get his perspective on the Dockerfile. In particular, curious if using multiple subsequent COPY commands is bad
| keyserver/Dockerfile | ||
|---|---|---|
| 9 ↗ | (On Diff #11354) | I'd do the update-alternatives in a separate RUN statement -- no need for them to be together |
| keyserver/docker-compose.yml | ||
| 1 ↗ | (On Diff #11354) | Version 1 is deprecated... should be "3" at least |
| 8 ↗ | (On Diff #11354) | Any reason the default container name isn't OK? It's probably "keyserver_node_1". |
| keyserver/Dockerfile | ||
|---|---|---|
| 9 ↗ | (On Diff #11354) | Okay, will separate. Just wondering – why is it that the other commands (apt-get-related) are better to keep together, but it's fine to have update-alternatives be separate? |
| keyserver/docker-compose.yml | ||
| 1 ↗ | (On Diff #11354) | This is really bad, not sure why I didn't just look at the version string in services/docker-compose.yml. That said, the docs are pretty confusing:
services/docker-compose.yml declares version 3.9, which I can't find anywhere in the docs. I did find this question, and one of the comments there references the Docker release notes to indicate 3.9 has been released. So I guess I'll use that... |
| 8 ↗ | (On Diff #11354) | Eh why not have something better? To be honest I included this based on a template somewhere, but I'm not a huge fan of the _1 suffix in the default that you quoted |
| keyserver/Dockerfile | ||
|---|---|---|
| 9 ↗ | (On Diff #11354) | Because the apt-get update adds a bunch of data to the apt cache and the rm removes it, so if these were separated you'd get a layer with the apt cache which you don't want. Basically, a RUN should do stuff and clean up after itself. update-alternatives isn't part of the cleanup so it can be separate. |
| keyserver/docker-compose.yml | ||
| 8 ↗ | (On Diff #11354) | Not a big deal, you can keep if you want but it's non-standard. And the default naming scheme lets you know which services are in the same docker-compose because they are grouped with the directory name. "keyserver_node" vs "node-keyserver". And in this case keyserver is a singleton so it makes sense that you'd drop the "_1" but it's part of the default name so you can scale up the number of containers per service. That's prob incompatible with port publishing though. |