feat: use convict for env vars

[harishv7]

Problem

We should have an exhaustive env variables check at service startup. This ensures that we can confidently use environment variables across our codebase without worrying about situations where the expected env var is missing or empty.

Solution

We can use the popular convict npm package which helps us create a schema of expected env vars and assert that these are present at system startup.

This introduces a configuration schema that ensure the environment is predictable and maintainable.

Do help to verify if all the env vars are captured well in the config file. There is one variable which me and @alexanderleegs were not able to identify the usage: AWS_BACKEND_EB_ENV_NAME used inlogger.js. This variable was not existing on prod EB env. When added to staging, it didn't make a change to the staging log-groups produced as well.

This has been excluded in the config. But we should decided if this variable is required or not.

Breaking Changes

• Yes - this PR contains breaking changes
  • Details ...
• No - this PR is backwards compatible

Tests

Do run npm run test --runInBand to ensure all the tests are passing

Deploy Notes

Ensure that all required variables are present in the environment files.
In the absence of the variables in .env the defaults will be used.

New environment variables:

Please check config.ts under src/config to ensure all the env variables match

New dependencies:

• "convict": "^6.2.4", : introduces a configuration schema that ensure the environment is predictable and maintainable

New dev dependencies:

• @types/convict : types for convict package

[seaerchin]

minor comments; on a separate note, do we need to define the required-string/number format cos convict uses the prototype to validate, which rejects null/undefined

[seaerchin]

also seems like got failing test; could i trouble you to look at it?

[kishore03109]

Chore: I think there are some parts of the code where there were some parsing logic for the env variables, which is now done by convict, and thus are no longer needed. (I got lazy to search through them towards the end HAHA)

Style: I think its cleaner to do a rebase rather than a merge to keep the commit changes clean, making the code easier to review! Let me know if you dont prefer this workflow!

[harishv7]

also seems like got failing test; could i trouble you to look at it?

fixed now

[seaerchin]

minor comments, not approving first for safety

[kishore03109]

mostly lgtm but a few comments.
I do have a concern due to the strictness setting that we have set for this. Currently, this will break if any one of our variables are set wrongly, causing prod to be down. Let me know your thoughts about this suggestion:

we can tackle this in 2 prs:

1. this PR, but mode is set to WARN rather than STRICT
   we look at the logs, make sure its not throwing any unexpected errors. The following week when we run release
2. we change the mode from WARN to STRICT

that way we ensure that things dont break, allowing for a smoother deployment.

[harishv7]

mostly lgtm but a few comments. I do have a concern due to the strictness setting that we have set for this. Currently, this will break if any one of our variables are set wrongly, causing prod to be down. Let me know your thoughts about this suggestion:

we can tackle this in 2 prs:

1. this PR, but mode is set to WARN rather than STRICT
   we look at the logs, make sure its not throwing any unexpected errors. The following week when we run release
2. we change the mode from WARN to STRICT

that way we ensure that things dont break, allowing for a smoother deployment.

Some clarifications:

• If any vars are set wrongly or not set, we will know at build time itself since app will not startup
• Prod envs should be set correctly, hence a "hard failure" should be expected when important things like env vars are missing/wrong

Basically when deploying, we should be 100% certain what we are deploying is correct, especially with things like config/envs

Did i get your concern correctly? But if the team feels we should not have a "hard" failure, we can discuss the deploy strategy as well

cc @seaerchin @alexanderleegs @QiluXie

[seaerchin]

mostly lgtm but a few comments. I do have a concern due to the strictness setting that we have set for this. Currently, this will break if any one of our variables are set wrongly, causing prod to be down. Let me know your thoughts about this suggestion:
we can tackle this in 2 prs:

1. this PR, but mode is set to WARN rather than STRICT
   we look at the logs, make sure its not throwing any unexpected errors. The following week when we run release
2. we change the mode from WARN to STRICT

that way we ensure that things dont break, allowing for a smoother deployment.

Some clarifications:

• If any vars are set wrongly or not set, we will know at build time itself since app will not startup
• Prod envs should be set correctly, hence a "hard failure" should be expected when important things like env vars are missing/wrong

Basically when deploying, we should be 100% certain what we are deploying is correct, especially with things like config/envs

Did i get your concern correctly? But if the team feels we should not have a "hard" failure, we can discuss the deploy strategy as well

cc @seaerchin @alexanderleegs @QiluXie

if convict fails at start up, i'm more in favour of failing fast! env vars are something that is difficult to debug/catch (even when warn), and we should try to fail at start up - that's also the reason why we do

if (!SOME_ENV_VAR) throw Error()

but because this fails when the file is used, it's still not as ideal. failing fast here has 2 benefits:

1. we can easily rollback to a working version whilst figuring out what's wrong
2. no tricky bugs involved (exact env var will be pointed out)

[kishore03109]

@harishv7

If any vars are set wrongly or not set, we will know at build time itself since app will not startup

Woah am confused about this since my understanding is that it will fail at runtime rather than build time, but ill clarify with you offline instead haha

Prod envs should be set correctly, hence a "hard failure" should be expected when important things like env vars are missing/wrong

Oh yes, agreed that this should be the end state. My concern primarily stemmed from the fact we are merging into develop, and develop and identity branch has diverged to some extent, I was worried that the merge itself will have breaking changes. But, I agree with @seaerchin about failing fast since we do have rolling deployments anyways, so prod wont be down even if the app fails at runtime.

TLDR: lgtm

[harishv7]

@harishv7

If any vars are set wrongly or not set, we will know at build time itself since app will not startup

Woah am confused about this since my understanding is that it will fail at runtime rather than build time, but ill clarify with you offline instead haha

Prod envs should be set correctly, hence a "hard failure" should be expected when important things like env vars are missing/wrong

Oh yes, agreed that this should be the end state. My concern primarily stemmed from the fact we are merging into develop, and develop and identity branch has diverged to some extent, I was worried that the merge itself will have breaking changes. But, I agree with @seaerchin about failing fast since we do have rolling deployments anyways, so prod wont be down even if the app fails at runtime.

TLDR: lgtm

@kishore03109 Yea, you're right, it is technically runtime, but at service startup.

@kishore03109 @seaerchin @alexanderleegs
We are actually merging into vapt branch first. Is that the right destination branch?
For deploy strategy, we need to deploy this on staging first. What would be the best way to do that?

[harishv7]

@harishv7

If any vars are set wrongly or not set, we will know at build time itself since app will not startup

Woah am confused about this since my understanding is that it will fail at runtime rather than build time, but ill clarify with you offline instead haha

Prod envs should be set correctly, hence a "hard failure" should be expected when important things like env vars are missing/wrong

Oh yes, agreed that this should be the end state. My concern primarily stemmed from the fact we are merging into develop, and develop and identity branch has diverged to some extent, I was worried that the merge itself will have breaking changes. But, I agree with @seaerchin about failing fast since we do have rolling deployments anyways, so prod wont be down even if the app fails at runtime.
TLDR: lgtm

@kishore03109 Yea, you're right, it is technically runtime, but at service startup.

@kishore03109 @seaerchin @alexanderleegs We are actually merging into vapt branch first. Is that the right destination branch? For deploy strategy, we need to deploy this on staging first. What would be the best way to do that?

Okay confirmed destination branch should be identity-phase-2 with @alexanderleegs