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

env-cmd fails if there is no .env file, should just give a warning #92

Open
Andre-H opened this issue Oct 1, 2019 · 24 comments

Comments

@Andre-H
Copy link

@Andre-H Andre-H commented Oct 1, 2019

Hi Todd.
If my project doesn't have a .env file, my command
env-cmd node etcetc
Will fail with:

Error: Unable to locate env file at default locations (./.env,./.env.js,./.env.json)
at getEnvFile (C:\workspace\so-ui-e2e-test\node_modules\env-cmd\dist\get-env-vars.js:34:11)

But look, I don't want to commit a .env file to my repository. Meaning when my CI clones and tries to run the project, there isn't a .env file there, and the CI build will fail.

Not having a .env file from which to load should be no worries, because I can
$PORT=1234 API_KEY=${secret_key} npm run start
in the CI environment.

But now you're making me commit my (empty) .env file so that CI doesn't fail, well it's only going to be a matter of time before someone commits their dev secrets to the repository.

IMHO you should allow the scripts to run if you don't find a .env file, just throw a warning with output to console, what do you think?

@mustafawm

This comment has been minimized.

Copy link

@mustafawm mustafawm commented Oct 7, 2019

Interesting!

I think the @toddbluhm's idea was/is to fail so it safeguards against missing files or wrong paths...etc.

But your scenario seems a valid one.

Maybe we add a flag --notRestrict to throw warnings instead of failing?

@westl

This comment has been minimized.

Copy link

@westl westl commented Oct 7, 2019

I ran into this problem as well recently.

As a workaround I just made another script command to run that has the "env-cmd" param specified.

It looks something like this.
"start":
"node dist/src"

"start:env":
"env-cmd node dist/src"

CI/CD will run the "start" which doesn't have the "env-cmd" parameter so you don't get an error or unpredictable environment variables on the server.

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Oct 30, 2019

I agree a flag to let it passthru without error would be very much appreciated.

I don't agree with creating another script, environment variables are designed to be optional, therefore the environment file should be too. I want to let anyone configure these variables either through a file or through different means, it should not change how I call my scripts whatsoever.

ProLoser added a commit to ProLoser/env-cmd that referenced this issue Oct 30, 2019
Fixes toddbluhm#92 by providing a passthru flag
@westl

This comment has been minimized.

Copy link

@westl westl commented Oct 31, 2019

@ProLoser I don't believe the thumbs down was warranted. My intention was to provide a workaround to unblock people on this issue until there's a working fix published. Sorry if my comment somehow looked like a solution instead of a workaround =). I see you created a PR good work!

@mustafawm

This comment has been minimized.

Copy link

@mustafawm mustafawm commented Oct 31, 2019

environment variables are designed to be optional

I disagree. That statement is too generic.

For example, if a front end build script had an environment variable to define the API URL, then env vars here are not optional. Actually It's better fail the build in this case.

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 1, 2019

Thanks for all the comments. This is a great discussion!

So I did some checking on previous versions and as of version 8.0.2:

  • It will not error when used with a missing .env file
  • It will error if the --fallback flag is used, but neither provided nor fallback .env files exist.
  • It will not error if a .rc environment is provided, but no .rc file actually exists
  • It will error if a .rc environment is provided, a .rc file exists, but the provided environment does not exist within the .rc file

So my conclusion is that env-cmd should not error if the .env file does not exist. This is a bug that probably snuck in during the conversion to typescript. This will need to be fixed, however, it will require a major version bump, because it will be a breaking change.

Also, a quick search through the issues will land you at this issue: #26 Seems like this is not the first time this project has encountered this bug.

@mustafawm

This comment has been minimized.

Copy link

@mustafawm mustafawm commented Nov 1, 2019

should not error if the .env file does not exist.

Won't this contradict #59 (v.9.0)?

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 1, 2019

I feel this should be a distinct argument that controls the failure behavior:

env-cmd --notFound=warn [--fallback] ...
env-cmd --notFound=error [--fallback] ...
env-cmd --notFound=silent [--fallback] ...

With the default value being warn? Open to better argument names / values tho.

Perhaps:

// warn
env-cmd [--fallback] ...
// error's out
env-cmd --fail [--fallback] ...
// mutes warning
env-cmd --silent [--fallback] ...
@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 1, 2019

@mustafawm you are absolutely correct! Not sure how I missed that issue!

When I first saw this discussion my gut reaction was to say, yeah it should by default fail if the .env file is not found. As a new user, I would be frustrated and confused why env-cmd was not setting env vars in my program only to find out later on that I had the wrong path and it was not finding my .env file.

But then I saw the earlier issue I pointed out and figured I must have made the decision to leave it as a non-error condition, clearly though I changed my mind in v9.

With all of that said, I think failing on a missing env file is the right course of action, especially for new users. I also agree that there should be some option to ignore that for power users.

I really like the --silent flag for muting failures, I feel like I have seen that used elsewhere too for similar behavior. I would prefer just one option/flag so let's go with --silent and all it does is silence failures for:

  • missing .env file
  • missing .rc file
  • missing environment in the .rc file
  • missing --fallback file

I think that should handle all use cases? No warning should be necessary either, just failure or no failure I think should be the conditions.

Thoughts or comments?

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 1, 2019

I'd really like a polite warning or something in the logs so I can confirm behavior.

Perhaps a --verbose flag that tells you which file was found or if it didn't find a file at all? I still don't want this to fail however.

Without any feedback, it will be harder to debug where things are going wrong with the --silent flag and it's still difficult to figure out WHERE it found a file, in case you are expecting one fallback to be used but instead a different one was used.

Honestly I prefer a warning by default (which is clear and actionable but not blocking) with options to mute it or fail on it.

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 1, 2019

I am certainly not opposed to a --verbose flag. I have thought about adding that for a while, but the need was never great enough and no one has really ever requested it.

I would, however, say that if you are concerned about it picking up the wrong file, you should not be using the --silent flag. Do your debugging and testing without the --silent flag so you get your failures, but then once you have everything fixed and running, add the --silent flag to get your final desired behavior.

Does that make sense?

@mustafawm

This comment has been minimized.

Copy link

@mustafawm mustafawm commented Nov 3, 2019

Isn't it considered verbose by default since it fails when it cannot find a file?

So --silent will just help suppressing that for power users. (👍).

Just an opinion:
I like the idea of console.info to tell the user whether the file was found or not.
When found, it tells the users "reading yourfile.env..."
When not found, it fails (current behavior)
When not found with --silent, it still tells the the users ".env file couldn't be found but we're honoring the --silent flag you passed..."

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 3, 2019

If you output logging it by definition is not silent.

I don't think pure silent is really useful though, I think you should always see what file was found, the only variation should be if it fails or not.

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 4, 2019

Alright so here is what my thinking/plan is so far.

Current style:

When you run env-cmd it does not output anything (just whatever the command your passing in outputs). The only time env-cmd outputs something is under a failure condition to help explain why the failure occurred. I believe that is reasonable so as to help debug the failure.

New style:

The above style will stay the same, with no changes.

A --verbose flag will be added. This will output a lot more debugging information to help users debug situations/issues and understand how env-cmd works.

A --silent flag will be added. This will silence all output from env-cmd (obviously whatever the command your passing will still output normally). It will also prevent failure conditions for the following:

  • Missing .env file
  • Missing .rc file
  • Missing environment within the .rc file

Failures that will still occur under the silent flag:

  • Bad .env file format/parsing errors
  • Bad .rc file format/parsing errors
  • If the passed in command fails, the failure will bubble up as expected and exit with the correct exit code

The --verbose flag will override the --silent flag as far as output is concerned. The --silent flag will still suppress errors for the above conditions.

@mustafawm

This comment has been minimized.

Copy link

@mustafawm mustafawm commented Nov 4, 2019

Failures that will still occur under the silent flag:

* Bad `.env` file format/parsing errors

* Bad `.rc` file format/parsing errors

* If the passed in command fails, the failure will bubble up as expected and exit with the correct exit code

I think if you pass --silent you should silence (just an opinion).

The --verbose flag will override the --silent flag as far as output is concerned. The --silent flag will still suppress errors for the above conditions.

Or just fail when both flags are provided, display help menu, ask user to pick one option. (just a thought)

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 4, 2019

@mustafawm I did give serious thought about --silent just preventing all errors (except ones bubbled up from the passed in command). I'm certainly not opposed to that and actually somewhat in favor of it, I was just concerted it might be too extreme. But on the other hand --silent is supposed to be a power user feature anyways.

Thanks for your thoughts on --verbose and --silent at the same time. I will think on that more but I still lean towards --verbose taking precedence. If you are passing the --verbose flag and you don't want output, then remove the --verbose flag. I guess at this point --silent is not so much about output as it is about suppressing errors.

With all of that said, will my proposed solution/thinking solve your issue? Because that is after all the point of this whole discussion, to solve real-world use case scenarios for everyone in this thread.

@mustafawm

This comment has been minimized.

Copy link

@mustafawm mustafawm commented Nov 4, 2019

Thanks mate.
Currently I'm not facing any issues with the current behavior (nor do I think the new behavior will cause any). The main issue here is OP's

I was reviewing this 🆒 package to see if there are new changes/maintenance issues..before re-using it in my current/latest project 🙂
So I came across this issue and thought I share my two cents.

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 5, 2019

Would really love to have some sort of conclusion to this discussion since I have an open PR just waiting to get merged and in the end I don't really care WHAT happens as long as I can see warnings when files are (and aren't) found

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 7, 2019

@ProLoser well then here is your conclusion.

--verbose will log info relating to what .env file and/or environment is being used regardless of any other flags as well as what settings were set. This flag is primarily used for debugging purposes.
--silent will prevent env-cmd from failing on any errors except those produced by the child process (the executing program).

Feel free to adjust your PR to this if you like. I can also start work on a PR sometime in the next few days if you would rather I implement this.

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 7, 2019

So if I want to get warnings without stopping on missing files I'd use both flags together?

env-cmd --verbose --silent yarn myScript?

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 7, 2019

@ProLoser Yes. --verbose won't be too crazy though. It should really only print options/flags set, location of env/rc file used (or missing) and potentially the environment if rc file used. So hopefully only three lines or so worst case scenario. Depending on how we format the flags/options.

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 7, 2019

Okay, I will TRY to start a new PR today if I have time, but I can't touch my existing PR since our internal code is pointing to that branch. If I don't open a PR within the next day or two though perhaps you can just tackle it?

@toddbluhm

This comment has been minimized.

Copy link
Owner

@toddbluhm toddbluhm commented Nov 15, 2019

Just PR'd the first part of this issue. #101 adds support for --verbose flag. I started here because it forced me to re-structure the errors and messaging to be more consistent and organized.

Once this is in, it should be much easier to create the --silent flag.

@ProLoser take a look at the logs being printed and let me know if they suffice for what you need.

@ProLoser

This comment has been minimized.

Copy link

@ProLoser ProLoser commented Nov 15, 2019

@toddbluhm the logs look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.