Details

scripts/set-admin-data.js
1	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: 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. 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	Should we do any error-checking here?

tomek added inline comments.Feb 15 2024, 3:43 AM

scripts/set-admin-data.js
17–18	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.

inka added inline comments.Feb 19 2024, 3:25 AM

scripts/set-admin-data.js
1	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	I don't think it's worth doing now, but if you feel strongly about this, let me know
33–57	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

Handle errors

Harbormaster completed remote builds in B26961: Diff 37320.Feb 19 2024, 3:46 AM

tomek accepted this revision.Feb 19 2024, 4:14 AM

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

inka added a child revision: D11104: [keyserver][lib] Make updates code aware that ids are strings, not numbers.Feb 19 2024, 4:52 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 ↗	(On Diff #37320)	Can you please strip this before landing?
17–18		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

inka added inline comments.Feb 20 2024, 12:22 AM

scripts/set-admin-data.js
1 ↗	(On Diff #37320)	This causes eslint errors
17–18		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