Page MenuHomePhabricator

[scripts] Create a script for configuring user credentials
ClosedPublic

Authored by inka on Feb 14 2024, 6:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 24, 6:05 AM
Unknown Object (File)
Tue, Dec 24, 6:05 AM
Unknown Object (File)
Tue, Dec 24, 6:05 AM
Unknown Object (File)
Tue, Dec 24, 6:05 AM
Unknown Object (File)
Tue, Dec 24, 6:05 AM
Unknown Object (File)
Tue, Dec 24, 6:04 AM
Unknown Object (File)
Tue, Dec 24, 6:04 AM
Unknown Object (File)
Fri, Dec 6, 7:40 PM
Subscribers

Details

Summary

issue: ENG-6615

Test Plan

I added node $PRJ_ROOT/scripts/set-admin-data.js to dev-shell.nix:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Feb 14 2024, 6:47 AM
scripts/set-admin-data.js
1 ↗(On Diff #37075)

I don't think Flow is actually checking this. We don't have a Flow workspace configured at the repo root.

We do already have four other JS scripts in this folder, all of which use the Flow pragma, and none of which are actually using Flow.

In fact I think if you tried to add Flow typehints, it would prevent this script from running directly with Node.

Two requests:

  1. Is there another place we could put this code so it would be typechecked? For instance, what if it was in the keyserver folder, and it ran automatically when the keyserver starts up and a dev environment is detected? This would replace the current dev-shell.nix approach, which won't work well with a transpilation requirement.
  2. If that doesn't work, can you strip the // @flow annotations here? Would be good to strip them from the other JS files in this folder as well.
33–57 ↗(On Diff #37075)

Should we do any error-checking here?

scripts/set-admin-data.js
17–18 ↗(On Diff #37075)

Should we hide the password in the console? Shouldn't be too hard https://stackoverflow.com/questions/24037545/how-to-hide-password-in-the-nodejs-console, but it's just for the local dev environment, so not necessarily worth it.

scripts/set-admin-data.js
1 ↗(On Diff #37075)

This would replace the current dev-shell.nix approach

We took this approach because the script will be taking the user input. And if I'm not mistaken, when the keyserver is run, a couple of threads run at one printing to the output, so that could cause problems.
I also need this to be run along with moving and creating a new db, which I think should be done as part of the nix scripts.

If that doesn't work, can you strip the // @flow annotations here? Would be good to strip them from the other JS files in this folder as well.

I would rather do this as a followup, because a couple of devs need to be able to use multiple keyservers, and I will be gone next week. I created a task in my March goal that is the second part of this testing goal, but if you think this is a priority, please let me know
ENG-6864

17–18 ↗(On Diff #37075)

I don't think it's worth doing now, but if you feel strongly about this, let me know

33–57 ↗(On Diff #37075)

I think if creation of these files fail, than nix develop should just fail, to force a re-run.
I should make the error more descriptive

This revision is now accepted and ready to land.Feb 19 2024, 4:14 AM

I would rather do this as a followup, because a couple of devs need to be able to use multiple keyservers, and I will be gone next week. I created a task in my March goal that is the second part of this testing goal, but if you think this is a priority, please let me know

This would take ~3min... I wonder if the amount of time you spent typing this message and creating the task, along with the time I am now spending typing this, is higher than the amount of time it takes you to simply put a diff up that strips the annotations. In this case I don't think the work should have been deferred. It's less a question of prioritization, and more a question of using your time and my time wisely.

scripts/set-admin-data.js
1

Can you please strip this before landing?

17–18 ↗(On Diff #37075)

It seems fairly trivial to do this now... but if there's a place that a follow-up task should have been created, it's definitely here. This is directly related to the project and would probably take you something more like 15-30min

This would take ~3min... I wonder if the amount of time you spent typing this message and creating the task, along with the time I am now spending typing this, is higher than the amount of time it takes you to simply put a diff up that strips the annotations. In this case I don't think the work should have been deferred. It's less a question of prioritization, and more a question of using your time and my time wisely.

Simply stripping the notations causes eslint errors, so I would have to figure out how to deal with that

scripts/set-admin-data.js
1

This causes eslint errors

17–18 ↗(On Diff #37075)

Here is the task: ENG-6874
I keep encountering unexpected problems this week, so I would rather not take on any work that isn't really necessary, since I will be gone next week, and some devs need this work to be able to test their code

Remove flow annotation. I was informed this is how this should be done, hope this is fine

Here is the diff with the remaining files: D11116

Hanlde folders not existing