Page MenuHomePhabricator

[keyserver] Call setupDB from DB migration if meta table is missing
ClosedPublic

Authored by ashoat on May 30 2022, 1:58 PM.
Tags
None
Referenced Files
F3500286: D4151.diff
Fri, Dec 20, 1:11 AM
Unknown Object (File)
Tue, Dec 17, 7:58 AM
Unknown Object (File)
Tue, Dec 17, 7:58 AM
Unknown Object (File)
Tue, Dec 17, 7:57 AM
Unknown Object (File)
Tue, Dec 17, 7:55 AM
Unknown Object (File)
Mon, Dec 16, 6:54 AM
Unknown Object (File)
Mon, Dec 9, 3:34 AM
Unknown Object (File)
Fri, Dec 6, 11:28 PM

Details

Summary

Depends on D4150

Linear task: ENG-1208

Test Plan

Create a keyserver/.env file, eg.:

COMM_MYSQL_DATABASE=commdev
COMM_MYSQL_USER=commdev
COMM_MYSQL_PASSWORD=pass

And then run the Docker image: cd keyserver && docker-compose down -v && docker-compose up --build

It should successfully connect to MySQL and start all of its threads. At this point (for the first time) Node should stop crash looping.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
keyserver/src/database/migrations.js
128 ↗(On Diff #13214)

It might be a good idea to explain what this magic number means

This revision is now accepted and ready to land.May 31 2022, 5:11 AM
atul added inline comments.
keyserver/src/database/migrations.js
123–128 ↗(On Diff #13214)

Yeah, agree. Based on a quick Google search it looks like 1146 ~= MySQL error code for table doesn't exist.

One option would be to leave a comment explaining that. Alternatively, might be good to define something like

const MYSQL_TABLE_DOESNT_EXIST_ERROR_CODE = 1466

somewhere in the file so the name is "self-documenting" and comments can be avoided.


Can name the constant anything, but would probably be good to name it in a way that it's clear the error code is "MySQL-specific" since "one day" it might change to MariaDB or Postgres or whatever?

keyserver/src/database/migrations.js
123–128 ↗(On Diff #13214)

Makes sense. For context, the full reference of MySQL 5.7 error codes is here.

Since we have this pattern in a couple of other places in the codebase, I've addressed this in a follow-up diff: D4157