search

nearform / titus

Deploy useful features in sprint one
https://nf-titus.netlify.app
Apache License 2.0
61 stars 43 forks source link

Invalid regexes used to check whether the back end and database manager services are running in production mode. #1481

Closed kurtmilam closed 2 years ago

kurtmilam commented 2 years ago

The regexes used to set config.isProduction in both services are invalid, always returning false, no matter what value NODE_ENV is set to in the respective .env file.

Here's the back end code:

https://github.com/nearform/titus/blob/71723b21dacdbd2229f5d6cd0cb10b0924d12e70/packages/titus-backend/lib/config/index.js#L54

And the database manager code:

https://github.com/nearform/titus/blob/743c044c8d7151e098af174f886c8ede141114f6/packages/titus-db-manager/lib/config.js#L22

I'm happy to open a PR to fix this, but I'd like to discuss the solution before I start. Just fix it separately in both services? Since titus is a monorepo project, it could be nice to have a utils type module that contains reusable functions, but that seems overkill for this bug.

I think my preference would be to fix it in both services, getting rid of the regexes and either:

  1. Being strict about the value - i.e. const isProduction = config.NODE_ENV === 'production'

  2. Or else be slightly less strict about the value, e.g. const isProduction = config.NODE_ENV.trim().toLowerCase() === 'production'

I prefer option # 1, since I'm pretty sure envSchema trims any whitespace from environment variables, anyway.

An in-between option (# 3) would be to expect whitespace to be correct, but use toLowerCase to be less strict about the capitalization.

hails commented 2 years ago

I've opened the PR with the fix, following your suggestion about being strict about the value. Also, I've opened an issue (#1483) where we can discuss the creation of a utils (or something alike) module.