Skip to content
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

✨ Add dotenv file hierarchy #200

Merged
merged 3 commits into from
May 27, 2020
Merged

✨ Add dotenv file hierarchy #200

merged 3 commits into from
May 27, 2020

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented May 27, 2020

Purpose

As discovered in #199 the Node dotenv package does not support dotenv file hierarchy out-of-the-box like dotenv-rails does. Since test commands are run as a child process of the percy command, the .env file has already been loaded into the environment via dotenv and will not be overridden by dotenv-rails.

Approach

The dotenv-rails readme outlines the order of priority for various supported files. Since dotenv will only set env vars that have not been previously set, we can load the files in priority order to mimic dotenv-rails hierarchy.

In addition to properly setting variables as expected to be set by dotenv-rails, this also makes it so various PERCY_* environment variables can be set in environment specific dotenv files as well.

The NODE_ENV environment variable must be set for environment specific dotenv files. For example, NODE_ENV=production will load .env.production.local, .env.local, .env.production, and .env files in that order.

Questions

As mentioned in #199, we might be wary of continuously adding support for different dotenv language behaviors. I personally like dotenv-rails approach and think it's fine to add this support so we can capture PERCY_* variables defined in those files. As for other languages, what are their approaches (if any) and should we include a way to disable loading .env files completely in this library?

@wwilsman wwilsman requested a review from Robdel12 May 27, 2020 19:08
@Robdel12
Copy link
Contributor

and should we include a way to disable loading .env files completely in this library?

Probably. I don't feel like it's a high priority, but I think it's the right thing to also offer.

Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏁 Good stuff, I dig it. We should document this somewhere too

@wwilsman wwilsman merged commit d8df115 into master May 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the ww/dotenv-hierarchy branch May 27, 2020 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants