Access Parameter store config locally via Dotenv #4745
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -944 B (0%) Total Size: 1.68 MB
ℹ️ View Unchanged
|
OllysCoding
reviewed
Apr 27, 2022
OllysCoding
reviewed
Apr 27, 2022
jamesgorrie
changed the title
mxdvl
reviewed
May 4, 2022
jamesgorrie
force-pushed
the
server-side-dotenv
branch
from
159af6c
to
523f13b
Compare
⚡️ Lighthouse report for the changes in this PRLighthouse tested 2 URLs Report for Article
Report for Front
|
jamesgorrie
force-pushed
the
server-side-dotenv
branch
2 times, most recently
from
fab115d
to
86d695d
Compare
jamesgorrie
force-pushed
the
server-side-dotenv
branch
from
0dcae01
to
9bbbf58
Compare
jamesgorrie
commented
May 4, 2022
jamesgorrie
commented
May 4, 2022
jamesgorrie
requested review from
a team,
JamieB-gu,
marsavar and
iainjchambers-guardian
as code owners
OllysCoding
reviewed
May 18, 2022
|
|
||
| ### Lines in the sand | ||
|
|
||
| #### `.env` shouldn't be required |
There was a problem hiding this comment.
+1 An important point not to be forgotten
There was a problem hiding this comment.
Do you think it's worth enforcing in some or other way? That might be how we access to config is the only way I can think of.
OllysCoding
reviewed
May 18, 2022
OllysCoding
reviewed
May 18, 2022
OllysCoding
reviewed
May 18, 2022
There was a problem hiding this comment.
This looks really good!
Should we document somewhere how this all works, and I think an important thing to cover is that right now we only pass our .env variables into the server build - meaning that they're not available on the client.
It's important that if that were to ever change appropriate considerations were taking (like requiring a CLIENT_ prefix) to avoid any accidental security leaks.
jamesgorrie
commented
May 18, 2022
| @@ -0,0 +1,81 @@ | |||
| const { CredentialsProviderError } = require('@aws-sdk/property-provider'); | |||
| const path = require('path'); | |||
| const { prompt, log, warn } = require('../env/log'); | |||
There was a problem hiding this comment.
I personally don't like the colours in this library, but at least using it will make things consistent.
There was a problem hiding this comment.
I didn't actually know this library existed! Are there some other places we should be using it?
There was a problem hiding this comment.
It’s been there … a while #80 😆
mxdvl
reviewed
May 18, 2022
Co-authored-by: Olly <9575458+OllysCoding@users.noreply.github.com>
…ring into server-side-dotenv
|
@mxdvl "floating promises" Could you point this out - if you're talking about in "common-js" <- same it was just copy/paste. @OllysCoding nice idea, I'll add an ADR. |
jamesgorrie
force-pushed
the
server-side-dotenv
branch
from
3bd6f1b
to
3c74346
Compare
mxdvl
approved these changes
May 20, 2022
ioannakok
reviewed
May 20, 2022
Co-authored-by: Ioanna Kokkini <ioannakok@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #4823
Generates a
.envfile from parameter store as part of ourmake devcommand.gen-dotenvhas been added to thebuild,build-cianddevtargets as it is only needed when running the server and is a file that is part of the build.I've added a check to the see if we're running
NODE_ENV=productionto fail if the file cannot be generated, and then set that for the general targetsbuildandbuild-cirather than just when running the node script..envDev messaging
Co-authored-by: Olly Namey olly.namey@guardian.co.uk
Co-authored-by: Ash Carr ashleigh.carr@guardian.co.uk
Co-authored-by: Ioanna Kokkini ioanna.kokkini@guardian.co.uk