Details

Reviewers

tomek
michal
ashoat

Commits

rCOMM239ee40c9665: [scripts] Add scrips for setting up a new authoritative keyserver

Summary

issues: ENG-6613 and ENG-6612
These scripts

check if usingIdentityCredentials flag is set to true in user_credentials.json
if so check if metadata table is correctly filled in
if either of these is not fulfilled asks the dev if they want to set up a new authoritative keyserver
if they agree, the db is moved to a backup and the dev is prompted for admin credentials (username and password)

I found using bash for operations on the db, and using js for writing files to be the most readable. I also had the js code already written as part of my February goal. But if this is too messy I can also write everything in bash. Please let me know what you think

In .sh files we don't seem to cut the lines to be under 80 characters

Test Plan

ran $PRJ_ROOT/scripts/set-up-authoritative-keyserver.sh from dev-shell.nix
ran nix and checked that if the dev aborts the process they can still use their old setup
ran from nix, and checked that if the dev agrees to proceed and gives correct credentials, the db is moved and user_credentials.json is filled in. It is also possible to run the keyserver and the db is correctly set up
ran from nix, and checked that if the dev agrees to proceed but gives incorrect credentials, then the keyserver fails. Then, the next time nix is run, the script correctly recognises that something is wrong, and asks again if the dev wants to set up the keyserver
(some logs explaining that nix has to be run again will be added in ENG-6618)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Remove comment

inka edited the summary of this revision. (Show Details)Mar 5 2024, 8:58 AM

Harbormaster completed remote builds in B27338: Diff 37856.Mar 5 2024, 9:35 AM

Harbormaster completed remote builds in B27339: Diff 37857.Mar 5 2024, 10:04 AM

inka requested review of this revision.Mar 5 2024, 10:04 AM

Great test plan!

scripts/set-up-authoritative-keyserver.sh
15 ↗	(On Diff #37857)	Typo here ("bakup")
18 ↗	(On Diff #37857)	Can you add a space after the `#` character?
19 ↗	(On Diff #37857)	Is there a way to make this part take two lines for readability? If not, let's at least add a space after the first semicolon
21 ↗	(On Diff #37857)	What happens if the user doesn't have a `comm` database?
21–22 ↗	(On Diff #37857)
28–40 ↗	(On Diff #37857)	You're accidentally using extra indentations here
33–34 ↗	(On Diff #37857)	Can you indent these lines?
41 ↗	(On Diff #37857)	Should there be a newline at the end of the file?
scripts/set-user-credentials.js
19 ↗	(On Diff #37857)	Will hiding the password be handled in a later diff?

This revision now requires changes to proceed.Mar 5 2024, 5:58 PM

inka added inline comments.Mar 6 2024, 3:27 AM

scripts/set-up-authoritative-keyserver.sh
21 ↗	(On Diff #37857)	If `comm` database is missing, an error is printed here, but the script continues. This script is supposed to be run from nix after "mariadb-up", so we don't ever expect the db to not be present. But if it isn't present, then this scrip has no additional negative effects on the setup - we just create an empty backup and set user credentials. But the fact that `comm` database doesn't exist is not relevant from the perspective of this script, I think. Since it's purpose can be expressed as "if comm database exists, move its contents, and save user credentials.". But I also don't want to silence the error, since it is useful to the dev to see that something most likely went wrong in the previous scripts. Please let me know if you disagree, and I should handle this error in a different way. Then I would also probably need to handle the `comm` user not existing.
scripts/set-user-credentials.js
19 ↗	(On Diff #37857)	Yes, this will be handled as part of ENG-6874

Address review

Harbormaster completed remote builds in B27357: Diff 37880.Mar 6 2024, 3:44 AM

tomek accepted this revision.Mar 6 2024, 3:47 AM

tomek added inline comments.

scripts/set-up-authoritative-keyserver.sh
21 ↗	(On Diff #37857)	Can we first check if `comm` DB exists and create `$db_version_name` DB only when the previous exists? Also, when the DB was just created by nix, we don't need to back it up, as it is empty. This could happen when a dev clones the repo and uses the new approach from the beginning.

inka added a child revision: D11258: [scripts] Hide password.Mar 6 2024, 6:27 AM

Agree with @tomek: we should not create a backup DB if there is no current DB, or if the current DB is empty. Assuming it's not too difficult, can you make this change?

scripts/set-up-authoritative-keyserver.sh
8 ↗	(On Diff #37880)	Nit: trailing space after `(y/N)`
11 ↗	(On Diff #37880)	Incorrect indentation here too. We shouldn't need to rely on diff review for this, as reviewers are unlikely to catch your indentation mistakes. Can you make sure to check your own indentation going forward?

This revision now requires changes to proceed.Mar 6 2024, 12:43 PM

inka added inline comments.Mar 8 2024, 9:08 AM

scripts/set-up-authoritative-keyserver.sh
8 ↗	(On Diff #37880)	This space is there so that user input is not concatenated with this text. Otherwise it would look like: Do you want to set up a new authoritative keyserver? (y/N)y instead of Do you want to set up a new authoritative keyserver? (y/N) y I think this should stay like that, but let me know if you disagree

Okay, that makes sense for the trailing space

Don't create backup if db not present or empty

scripts/set-up-authoritative-keyserver.sh
11 ↗	(On Diff #37880)	Sorry about this!

Harbormaster completed remote builds in B27448: Diff 37997.Mar 11 2024, 9:38 AM

ashoat accepted this revision.Mar 11 2024, 4:22 PM

ashoat added inline comments.

scripts/set-up-authoritative-keyserver.sh
21 ↗	(On Diff #37997)	Nit: let's be consistent about `[[ ]]` instead of `[ ]` when possible
40 ↗	(On Diff #37997)	I wonder if this log if necessary. Seems like it will always be printed when running `nix develop`