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

Addressing some issues around dotenv #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

motdotla
Copy link

@motdotla motdotla commented Sep 5, 2022

Hi @goldbergyoni 👋,

Very cool project you are putting together here. It will really help devs!

But I wanted to address some concerns I had about the writing concerning .env files. I've put that in this PR as a work in progress.

I actually think a developer would be MORE likely to endanger their secrets with convict. The pattern of adding .env to your .gitignore file is well established - often built into frameworks. That same pattern is not well established for a config.js file. A less experienced developer is likely to forget to add config.js to their .gitignore file - leaking their secrets. Most of all, GitHub likely won't trigger a warning to the developer when they push up this config.js like they do for a .env file.

Additionally, some of the arguments for sharing the .env file seem unfairly attributed to .env files. Sharing a config.js file securely between your team has the same problem.

The types issue makes sense. That is a limitation of .env files.

@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Base: 89.21% // Head: 90.43% // Increases project coverage by +1.22% 🎉

Coverage data is based on head (88e34ff) compared to base (c9ce577).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
+ Coverage   89.21%   90.43%   +1.22%     
==========================================
  Files          16       14       -2     
  Lines         482      460      -22     
  Branches       38       36       -2     
==========================================
- Hits          430      416      -14     
+ Misses         51       44       -7     
+ Partials        1        0       -1     
Flag Coverage Δ
app 90.43% <ø> (ø)
generator ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/code-generator/error-handling.ts
src/code-generator/index.ts

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@goldbergyoni
Copy link
Contributor

goldbergyoni commented Sep 8, 2022

@motdotla First, welcome aboard, and thanks for bringing a disruptive opinion. This is what we need here.

Obviously there is no clear cut here, we deal with predicting developers behaviours. I wish I could run an experiment but we have no data, only common sense. With that said, here is why I believe dotenv encourages leaking secrets more than other approaches. We deal with two different type of artifacts:

  • Value-based configuration - Whether dotenv or config.js, these artifacts are all about storing values. One value per key. Since this library is reading the config from this file (the env var are set based on the file content), it makes sense to put environmental data within this file. There is no other alternative.

  • Schema-based configuration - Convict and others are centered around metadata, not necessarily through concurrent data. It explicitly has a 'default' attribute to reflect what type of values might exist here (I use this as the values for my testing), it also has an attribute that describes the ENV_VAR name - Which explicitly tells the developer, YOU must somehow inject this value through some deployment mechanism. convict is not going to set this value for you (unlike dotenv), you have to find the right mechanism for env vars/secrets

To my humble opinion, the later will push more developers to store their secrets in a different system that inject them into the ENV VAR

WDYT?

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.

None yet

2 participants