yarn cleaninstall from Docker image
ClosedPublic
Actions

Authored by ashoat on Apr 12 2022, 8:27 AM.

Details

Reviewers

tomek
• jim

Commits

rCOMMca61a68e4d26: yarn cleaninstall from Docker image

Summary

For proper build caching, we want to first handle the node_modules, and then later copy in the actual files.

Depends on D3712

Test Plan

Make sure I can build the Node.js keyserver from Docker

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Apr 12 2022, 8:27 AM

Herald added subscribers: • abosh, • benschac, • karol and 2 others. · View Herald TranscriptApr 12 2022, 8:27 AM

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

Harbormaster completed remote builds in B8126: Diff 11354.Apr 12 2022, 8:37 AM

ashoat requested review of this revision.Apr 12 2022, 8:37 AM

tomek accepted this revision.Apr 15 2022, 8:43 AM

• jim requested changes to this revision.Apr 18 2022, 5:15 AM

• jim added inline comments.

keyserver/Dockerfile
9	I'd do the `update-alternatives` in a separate RUN statement -- no need for them to be together
keyserver/docker-compose.yml
1	Version 1 is deprecated... should be "3" at least
8	Any reason the default container name isn't OK? It's probably "keyserver_node_1".

This revision now requires changes to proceed.Apr 18 2022, 5:15 AM

ashoat added inline comments.Apr 18 2022, 9:56 PM

keyserver/Dockerfile
9	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	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: The Compose spec merges the legacy 2.x and 3.x versions, aggregating properties across these formats and is implemented by Compose 1.27.0+. `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	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

Review

Harbormaster completed remote builds in B8284: Diff 11580.Apr 18 2022, 10:09 PM

• jim requested changes to this revision.Apr 18 2022, 11:51 PM

• jim added inline comments.

keyserver/Dockerfile
9	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	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.