Page MenuHomePhabricator

Add table for keyserver olm accounts
ClosedPublic

Authored by marcin on Apr 21 2023, 4:22 AM.
Tags
None
Referenced Files
F1668222: D7560.id25591.diff
Fri, Apr 26, 5:37 PM
F1665029: D7560.id26264.diff
Thu, Apr 25, 8:58 PM
Unknown Object (File)
Thu, Apr 25, 6:57 PM
Unknown Object (File)
Sun, Apr 21, 9:33 PM
Unknown Object (File)
Sat, Apr 20, 12:58 AM
Unknown Object (File)
Tue, Apr 16, 12:16 PM
Unknown Object (File)
Tue, Apr 16, 11:41 AM
Unknown Object (File)
Tue, Apr 16, 11:41 AM
Subscribers

Details

Summary

This differential adds new table to MariaDB database on the keyserver to store both primary and notifications olm accounts.

Test Plan

To test migrations-config.js restart your keyserver terminal. Ensure DB version: 32 log line is visible.
To test setup-db.js create new database and update secrets folder so that it points to new database instance. Start your keyserver.

In both cases using a database connection tool of choice (I prefer to use mariadb terminal command), ensure keyserver_olm_accounts table is present and its structure matches SQL statement used to create it.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.Apr 21 2023, 7:05 AM
ashoat added inline comments.
keyserver/src/database/migration-config.js
351 ↗(On Diff #25514)

Can we just name it olm_accounts?

355 ↗(On Diff #25514)

Let's use latin1 for consistency

This revision now requires changes to proceed.Apr 21 2023, 7:05 AM
keyserver/src/database/setup-db.js
388 ↗(On Diff #25514)

I think this will make it unique on is_primary, but I think this is what we want

keyserver/src/database/setup-db.js
388 ↗(On Diff #25514)

It was concluded on crypto sync that we will ever have only two olm accounts on the keyserver - one primary and one for the notifs so it was intentional.

Change character set. Rename table. Fox formatting

Not clear why pickling_key and pickled_olm_account can be NULL... I think we should switch them to NOT NULL before landing

keyserver/src/database/migration-config.js
353–354 ↗(On Diff #25591)

Why is it possible for these to be NULL?

This revision is now accepted and ready to land.Apr 24 2023, 6:15 AM
keyserver/src/database/migration-config.js
354 ↗(On Diff #25591)

Also please keep lines to a max of 80 chars

keyserver/src/database/setup-db.js
261 ↗(On Diff #25591)

Please keep lines to a max of 80 chars

Add NOT NULL check. Rename is_primary to is_content

Once again – please pay attention to the feedback about line length! It's not a good use of my time (or your time) to have to go back-and-forth about this. Please address it BEFORE putting up diffs in the future

keyserver/src/database/migration-config.js
378 ↗(On Diff #25998)

Continuing to see issues with line length...

Comply to line length convention

Rebase to fix invalid miogrations versions

Rebase to fix an SQL syntax error

Rebase master before landing

This revision was automatically updated to reflect the committed changes.