-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
restore process.env.MONGODB_URI for test-project #1212
Conversation
🦋 Changeset is good to goLatest commit: c8b9578 We got this. Not sure what this means? Click here to learn what changesets are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
One comment, and needs a Changeset too.
test-projects/basic/.env.example
Outdated
@@ -2,3 +2,4 @@ | |||
# CLOUDINARY_KEY=abc123 | |||
# CLOUDINARY_SECRET=abc123 | |||
PORT=1 | |||
MONGODB_URI=mongodb://127.0.0.1:27017/tp_basic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add the value here, we don't want to force its usage, instead we can rely on the fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think providing this here provides a good example for users to follow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dotenv-safe
will read .env.example
for which env vars are required. With this change, if the environment doesn't provide a MONGODB_URI
, dotenv-safe
will throw an error, which is not what we want (we want it to default to the localhost fallback when not set), especially as this is a test project and we will be using an in-memory mongo server which is passed in manually (ie; not via an env var)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and removed.
6f85fc3
to
c8b9578
Compare
@jesstelford as discussed, #1201 did not introduce
CONNECT_TO
as a Keystone-wide environment variable, but only for thekeystone
command. Thereby this had nothing set.This resolves that, possibly.