[risk=no] Refactor docker-compose to leverage anchors #3658
Conversation
There was a problem hiding this comment.
Maybe comment on what the test plan/process was a bit.
I'd also really like to see (or I can do this too) a description associated with each runnable arg. I didn't see a way to do this in the docs yet (e.g. for printing tasks a la gradle), but we could use a fake tag like x-aou-description, or just a comment I guess.
| @@ -1,16 +1,22 @@ | |||
| version: "3" | |||
| version: "3.4" | |||
There was a problem hiding this comment.
Is the version change necessary for the other changes? If not, it might be nice to test that independently before or after.
There was a problem hiding this comment.
It's needed to have an x- property that docker-compose doesn't complain about. I will add a comment about that below. It's a minor version upgrade, and this file is only applicable to local command lines, so I don't think it adds much value to split out a PR for this.
| @@ -1,16 +1,22 @@ | |||
| version: "3" | |||
| version: "3.4" | |||
| x-api-defaults: &api-defaults | |||
There was a problem hiding this comment.
I've started to use workbench-api instead of api for clarity, as pretty much everything has an api.
Also, it sounds like this anchor is more generic than just API tasks, so maybe there's a more generic name. Not a huge deal, but it can be difficult to get your bearings here if you haven't used docker-compose much.
There was a problem hiding this comment.
I think this should be pretty contextually obvious in this case and it's symmetric to the directory name, I don't think the increased verbosity is worth it.
| - ~/.config:/.config:cached | ||
| - ~/.gsutil:/.gsutil:cached | ||
| command: | | ||
| cloud_sql_proxy |
There was a problem hiding this comment.
I do use cloud_sql_proxy locally with the, but I've never done so from docker-compose. @freemabd set me up with a named account on the test db, and I've been doing the same command locally to connect to it in IntelliJ:
./cloud_sql_proxy -instances all-of-us-workbench-test:us-central1:workbenchmaindb=tcp:0.0.0.0:3307 -credential_file=/Users/jaycarlton/repos/workbench/api/sa-key.json
Of course, as written this wouldn't work locally, unless I want to mount /w.
There was a problem hiding this comment.
I also use cloud_sql_proxy to connect to test mysql db when using sequelPro/intellij
| - gradle-cache:/.gradle | ||
| - ~/.config:/.config:cached | ||
| - ~/.gsutil:/.gsutil:cached | ||
| <<: *api-defaults | ||
| db: |
There was a problem hiding this comment.
can we name this workbench_app_db instead, since we're a multi-db project? I feel like we're starting to need a distinct name for the MySQL DB, like "Workbench Backend DB" or "Application DB". Though maybe it's obvious that we're not launching BQ databases inside Docker.
I suppose that would break compatibility and require doc updates.
There was a problem hiding this comment.
There's only one database here, and it feels like a pretty big stretch to think this could be referring to BigQuery. Is there a specific use case you are concerned about? Just reading this file?
There was a problem hiding this comment.
Fair enough. Just seems like a database should have a name other than database. The confusion isn't so much with this file or even with developers, but when discussing application components with product folks or other stakeholders.
| user: ${UID} | ||
| working_dir: /w/api | ||
| environment: | ||
| - GOOGLE_APPLICATION_CREDENTIALS=/w/api/sa-key.json |
There was a problem hiding this comment.
Can we simply include this line in the envs file?
There was a problem hiding this comment.
This doesn't relate to the database, so I don't think that would be correct. It also refers to a specific mounted path within the docker container.
| working_dir: /w/api/db | ||
| volumes: | ||
| - src-sync:/w:nocopy | ||
| - gradle-cache:/.gradle | ||
| entrypoint: ['with-uid.sh', 'wait-for-it', 'db:3306', '-t', '30', --] |
There was a problem hiding this comment.
Maybe parameterize the port number and timeout?
There was a problem hiding this comment.
See above comment. Also the port is fine, but the timeout is a constant. Creating knobs for things that don't need to change generally just adds noise. If the intent was to avoid duplication with the other services here, see the PR description: my preference is to just eliminate the services with duplicate functionality. If there's a legitimate need for multiple services with this entrypoint, using an anchor can also work to avoid double-encoding of this.
| environment: | ||
| - GOOGLE_APPLICATION_CREDENTIALS=/w/api/sa-key.json | ||
| env_file: | ||
| - db/vars.env | ||
| ports: | ||
| - 127.0.0.1:8081:8081 |
There was a problem hiding this comment.
Maybe make a named constant for the two different port numbers 8081 and 8001. If order is arbitrary, maybe list them alphabetically.
There was a problem hiding this comment.
Looks like the way to do this in docker-compose would be to push the ports into a .env file. local constants/variables don't seem to be possible in docker-compose, except maybe by further abusing anchors.
AFAICT the suggestion isn't really related to the PR, so I don't intend to make a non-trivial change like that here.
| @@ -20,165 +26,52 @@ services: | |||
| ports: | |||
| - 127.0.0.1:3306:3306 | |||
| api: | |||
| <<: *api-defaults | |||
There was a problem hiding this comment.
I'm a bit surprised you need to use an anchor. I would've expected docker-compose to have a high-level concept for this kind of reuse.
There was a problem hiding this comment.
Agreed, they used to have inheritance in docker-compose 2 which I initially tried to use here, but apparently they removed in v3. See moby/moby#31101
|
Puppeteer e2e is broken due to https://precisionmedicineinitiative.atlassian.net/browse/RW-5080 . Merging |
Behavioral change:
Manually tested:
./project.rbcommands with coverage of the various docker-compose servicesFuture work: