Page MenuHomePhabricator
Differential D5494

[sql/keyserver] removed/edited columns affected by SIWE
ClosedPublic
Actions

Authored by • derek on Oct 27 2022, 11:02 AM.
Tags
None
Referenced Files
F3248360: D5494.diff
Fri, Nov 15, 9:35 AM
Unknown Object (File)
Thu, Oct 17, 9:01 AM
Unknown Object (File)
Thu, Oct 17, 9:01 AM
Unknown Object (File)
Thu, Oct 17, 9:01 AM
Unknown Object (File)
Thu, Oct 17, 9:01 AM
Unknown Object (File)
Thu, Oct 17, 8:55 AM
Unknown Object (File)
Oct 9 2024, 11:39 PM
Unknown Object (File)
Sep 26 2024, 7:41 PM
View All Files
Subscribers
• abosh
ashoat
atul
ginsu
tomek
varun

Details

Reviewers
varun
tomek
atul
Commits
rCOMMce520b5a419c: [sql/keyserver] removed/edited columns affected by SIWE
Summary

as per title we're moving the public_key to the cookies column so i dropped those columns from users and sessions.
linear task
another linear task

Test Plan

run the migration script & watch some columns disappear

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• derek created this revision.Oct 27 2022, 11:02 AM
Herald added a subscriber: • abosh. · View Herald TranscriptOct 27 2022, 11:02 AM
• derek edited the summary of this revision. (Show Details)Oct 27 2022, 11:03 AM
• derek edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B13082: Diff 17960.Oct 27 2022, 11:16 AM
• derek requested review of this revision.Oct 27 2022, 11:16 AM
tomek requested changes to this revision.Oct 28 2022, 3:05 AM

we're moving the public_key to the cookies column

Is this value empty for all the existing rows? If that's not the case, we should migrate it, but otherwise we're fine.

run the migration script & watch some columns disappear

How did you run the script? Was it manually or automatically?
Also, the test plan should include a check of creating a new database - after uninstalling the app.

keyserver/src/database/migration-config.js
55–56 ↗(On Diff #17960)

Why do we use semicolon after every line? Is it a correct syntax? In other ALTER TABLE statements we're using commas to separate.

This revision now requires changes to proceed.Oct 28 2022, 3:05 AM
ashoat added inline comments.Oct 28 2022, 8:16 AM
keyserver/src/database/migration-config.js
55–56 ↗(On Diff #17960)

I suspect this is invalid syntax, which makes me question whether it was tested at all...

• derek updated this revision to Diff 17965.Oct 28 2022, 9:13 AM

fixed syntax

• derek marked 2 inline comments as done.Oct 28 2022, 9:15 AM
• derek added inline comments.
keyserver/src/database/migration-config.js
55–56 ↗(On Diff #17960)

my mistake, i had merge conflicts when i rebased & i must've missed this

Harbormaster completed remote builds in B13087: Diff 17965.Oct 28 2022, 9:27 AM
tomek accepted this revision.Nov 2 2022, 7:13 AM
This revision is now accepted and ready to land.Nov 2 2022, 7:13 AM
Closed by commit rCOMMce520b5a419c: [sql/keyserver] removed/edited columns affected by SIWE (authored by • derek). · Explain WhyNov 2 2022, 8:34 AM
This revision was automatically updated to reflect the committed changes.
• derek marked an inline comment as done.
• derek added a commit: rCOMMce520b5a419c: [sql/keyserver] removed/edited columns affected by SIWE.
• derek added a reverting change: D5519: Revert "[sql/keyserver] removed/edited columns affected by SIWE".Nov 2 2022, 10:50 AM
• derek mentioned this in D5521: [sql/keyserver] added multipleStatements to fix fatal error.Nov 2 2022, 11:18 AM
ashoat mentioned this in rCOMM6e570829c37c: [sql/keyserver] added multipleStatements to fix fatal error.Nov 3 2022, 11:57 AM

Revision Contents
Changeset List

PathSize
keyserver/
src/
database/
13 lines
14 lines

Diff 18021

View Options

keyserver/src/database/migration-config.js

Loading...
View Options

keyserver/src/database/setup-db.js

Loading...