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

Disable in CI environments #34

Closed
zeke opened this issue Jul 19, 2018 · 7 comments
Closed

Disable in CI environments #34

zeke opened this issue Jul 19, 2018 · 7 comments

Comments

@zeke
Copy link

zeke commented Jul 19, 2018

Hello, I'm an avid user of this module. Thanks for making and maintaining it.

I often find that this module causes issues in CI environments where the env configuration is not always the same as development. So our team (@electron) usually ends up doing this:

if (!process.env.CI) require('dotenv-safe').load()

All the CI providers set this environment variable.

Would you open to making this a built-in feature in dotenv-safe? Maybe an option?

Edit: One could even argue that because this tool is intended for use in development, it could automatically abort if process.env.CI is detected. That would of course be a breaking change and would require a major version bump.

@rolodato
Copy link
Owner

Hey @zeke, really glad to see that Electron is using this library :)

Could you go into a bit more detail about the issues you're seeing in CI? If you're passing in your environment variables directly to the process through CI and not from a .env file it should still work, provided you're passing in all the variables defined in .env.example:

it('does not throw error when .env is missing but variables exist', () => {
process.env.HELLO = 'WORLD';
assert.isOk(dotenv.load({
example: 'envs/.env.noDotEnv'
}));
});

@zeke
Copy link
Author

zeke commented Jul 20, 2018

Could you go into a bit more detail about the issues you're seeing in CI?

We have numerous CI environments with different jobs (Travis, AppVeyor, Circle, and VSTS), and some of them only need a subset of the variables listed in .env. For example, some boxes do releases to S3 and GitHub, so they need those credentials, but other boxes just run tests and don't need any of these credentials.

As an alternative to changing the behavior, we could also document the process.env.CI technique in the README. 🤔

@rolodato
Copy link
Owner

rolodato commented Jul 21, 2018

I see, that makes sense. I can't think of a sensible default that would work for both cases, where you either care or don't care about validating environment variables when running on CI. It's also problematic because you could still care about the subset of the variables you're using only for CI.

One workaround I can think of is using different example files depending on process.env.CI, something like this:

require('dotenv-safe').config({
  example: process.env.CI ? '.env.ci.example' : '.env.example'
});

which would have the benefit of "documenting" what's needed for CI and what isn't. You would probably have some duplication between the CI and non-CI example files, though.

I've also thought of accepting multiple example files, so you could have something like this:

require('dotenv-safe').config({
  example: process.env.CI ?
    ['.env.credentials.example', '.env.dev.example'] :
    ['.env.dev.example']
});

which would validate against the union of all variables defined in all the different files, and not need to have duplicated variables defined in each example file. Creating a single .env file from all of them could be done with cat .env.* | uniq > .env, which is a bit less elegant than cp .env.example .env. If there are duplicates it won't be a problem, because there can't be any conflicts. What do you think?

@zeke
Copy link
Author

zeke commented Jul 21, 2018

I think your first example is easier to understand. There could be duplicates across files but I think it's worth it to keep the API simpler.

@zeke
Copy link
Author

zeke commented Jul 21, 2018

Also thanks for taking the time to think through this problem. Just want to reiterate that supplementary documentation may be the best answer for now.

@rolodato
Copy link
Owner

You're welcome! Yes, I agree that adding documentation is the way to go. If more cases like this one come up, we can revisit adding new features to the API. Thanks for bringing this up, by the way.

@zeke
Copy link
Author

zeke commented Jul 22, 2018

Was just about to update the README but I see you did that already in 2d3e705. Thanks!

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

No branches or pull requests

2 participants