Add config & logger utils #1
Conversation
nonword
requested review from
7emansell,
charmingduchess,
danamansana and
yossariano
charmingduchess
left a comment
charmingduchess
left a comment
There was a problem hiding this comment.
A couple small notes. Would you also be open to figuring out how to publish this as both CJS and ESM compatible?
lib/kms.js
Outdated
| try { | ||
| response = await client.send(command) | ||
| } catch (e) { | ||
| throw new KmsError(`${e.name} during decrypt command`, { cause: e }) |
There was a problem hiding this comment.
Can we put the recommendation to use AWS_PROFILE=nypl-digital-dev in this error message? these errors will be logged in other repos, and the readme recommendation won't be easily accessible
There was a problem hiding this comment.
Added code to identify auth error and suggest using the env var.
lib/config.js
Outdated
| * | ||
| * @return {Promise<object>} An object with all decrypted config | ||
| * **/ | ||
| const loadConfig = (envName = process.env.ENVIRONMENT || 'production') => { |
There was a problem hiding this comment.
I think we should not have a default here. I can see cases like running a bulk index script where a bad path could cause a premature update to production data.
There was a problem hiding this comment.
Good call. Env name is now required.
Open to that. At a glance it seems that might pair well with doing a basic conversion to typescript, since it looks like dual support may require distinct builds anyway. I'll look into it, but it might need to be a separate effort. |
Initial implementation of common Node config loader and logger
https://newyorkpubliclibrary.atlassian.net/browse/SCC-4788