issue: ENG-7137
If the dev is using the new multikeyserver flow, they had provided some credentials during when nix was run. If registration with these credentials failed, they neeed to rerun nix, so that they can provide new credentials, but also so that the db gets fixed.
Details
Tested that if registration fails, and the flag is set, the warning is printed
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
keyserver/src/user/login.js | ||
---|---|---|
149 ↗ | (On Diff #37918) | Is the === true part really necessary? |
150–155 ↗ | (On Diff #37918) | Same comment as here: https://phab.comm.dev/D11267#inline-68282 |
151 ↗ | (On Diff #37918) | (Might need to split lines again to maintain 80 chars) |
152 ↗ | (On Diff #37918) |
Should we only print this log in dev mode?
It looks like the way we check if this is dev mode is by checking process.env.NODE_ENV === 'development'. But docker keyserver prints undefined as the value of process.env.NODE_ENV. Would checking process.env.NODE_ENV !== 'production' make sense? Is the real production keyserver set up to have process.env.NODE_ENV == 'production'?
But docker keyserver prints undefined as the value of process.env.NODE_ENV
Hmm... we shouild probably should avoid this ambiguous situation, and make sure we always specify NODE_ENV in the Docker environment. Can you create a task?
Would checking process.env.NODE_ENV !== 'production' make sense? Is the real production keyserver set up to have process.env.NODE_ENV == 'production'?
It doesn't look like I have anything specified in the .env file. If you're correct that the default keyserver is to have undefined, then I'm guessing my keyserver in production also has undefined. If it's helpful, I can do some additional testing on this.
In general the keyserver run in Docker is always a production keyserver. And I think we want this warning to show up, since it means that the keyserver is set up to use the multikeyserver flows, but something went wrong and some action is required to fix this state.
we shouild probably should avoid this ambiguous situation, and make sure we always specify NODE_ENV in the Docker environment
Since the Docker keyserver is always a prodution keyserver, does that make sense? I feel like we should either decide that production keyservers don't have the value set up, or specify it as 'production' for all production keyservers
In general the keyserver run in Docker is always a production keyserver. And I think we want this warning to show up, since it means that the keyserver is set up to use the multikeyserver flows, but something went wrong and some action is required to fix this state.
I don't think it makes sense to print this particular warning language in the Docker environment, since we aren't using nix develop there. I'm open to printing a warning, but I think it should make sure where it's printed.
I feel like we should either decide that production keyservers don't have the value set up, or specify it as 'production' for all production keyservers
I'm okay with either solution, but if you opt for the first one ("decide that production keyservers don't have the value set up"), we should go through places where we check that NODE_ENV is 'production'. In those case, we likely need to be checking that NODE_ENV is not set OR it's 'production'. Similarly, we should consider places where we check that NODE_ENV is NOT 'production', in which case we should check whether NODE_ENV is set AND it's NOT 'production'.
git grep NODE_ENV | grep production could be a good place to start this search:
- The process.env.NODE_ENV !== 'production' check in createAsyncMigrate can arguably be removed entirely... it seems superfluous
- I'm not sure about the process.env.NODE_ENV check in web/push-notif/service-worker.js. This appears to occur on web, but our Webpack config seems to make sure that NODE_ENV is always either 'production' or 'development'
- The other results don't appear to be relevant
I created a task to update NODE_ENV usage: ENG-7195
I don't think it makes sense to print this particular warning language in the Docker environment, since we aren't using nix develop there. I'm open to printing a warning, but I think it should make sure where it's printed.
I see, I will only print the "nix" part in dev mode then
keyserver/src/user/login.js | ||
---|---|---|
163 ↗ | (On Diff #38020) | Production keyserver can still be in non-Docker, right? Can the language also give guidance for the non-Docker environment? ("re-enter nix develop") |