support --env-file CLI option (#274)

[hamza512b]

I filing pr after I looked at this discussion #354.

This pr add support for ability to use different env files than the default one .env

[dossy]

In addition to my other review comments, there needs to be an addition to the README.md documenting this new Command Line Option.

[hamza512b]

I have applied the suggestions, however I came up with an issue, if we pass multiple .env files to the function loadDotEnv (there is no way to do it right now) and some file does not exists how should we handle that?

For now, what I did is return if any file does not exist.

[dossy]

I have applied the suggestions, however I came up with an issue, if we pass multiple .env files to the function loadDotEnv (there is no way to do it right now) and some file does not exists how should we handle that?

For now, what I did is return if any file does not exist.

We can rely on godotenv's underlying behavior (see readFile), where it will return an error if a file cannot be opened, such as when it does not exist or when the process lacks sufficient permission to read the file.

Go's os.Open will return an error if the file does not exist, however, and as we want to not halt execution in this case, we need to check the error condition for os.ErrNotExist and ignore it if that's the case, otherwise process the error as normal. This way, if the .env file, or alternative file specified with --env-file, does not exist, we currently won't raise an error, and simply continue, which is equivalent to the file existing but being empty, anyway.

[dossy]

if you want to support being able to load multiple environment files using repeated --env-file arguments, take a look at the --migrations-dir flag:

• How to specify the default value
• How to retrieve the array of values

Since loadDotEnv() already takes an array of strings as its parameter, supporting multiple --env-files is quite easy.

Also, since we're now not only loading .env, maybe it makes sense to rename loadDotEnv to loadEnvFiles? What do you think?

It would be nice to have a few tests covering this functionality. Is that something you'd like to try and add? If not, I can.

[hamza512b]

While implementing support for --env-file to accept multiple files, I have found an issue with approach I went with. We can not use app.Before to load environment variables, because urfave/cli will no longer be able use environment variables, because it apparently tries to run before function after it had evaluated the command-line arguments.

One way to deal with this is to manually parses the command-line arguments.

[dossy]

While implementing support for --env-file to accept multiple files, I have found an issue with approach I went with. We can not use app.Before to load environment variables, because urfave/cli will no longer be able use environment variables, because it apparently tries to run before function after it had evaluated the command-line arguments.

One way to deal with this is to manually parses the command-line arguments.

I suspect this is exactly why loadDotEnv() was called from main originally.

That's probably the simplest solution, here.

Oh, but then we don't get the benefit of urfave/cli parsing the command line args for us? Maybe we can actually use it twice in the same program, once to parse just the --env-files arguments, and then a second time for the actual program?

I'll experiment a bit and report back if it works.

[dossy]

Okay, so urfave/cli uses the Go flag package for its argument parsing, and it returns a "flag provided but not defined" error and that's not configurable, so it interrupts flag parsing if an unrecognized flag is present, which we don't want.

In other words, --env-file .env.local --foo --env-file .env.development.local will only parse the --env-file .env.local and return an error when it reaches --foo (as long as it's not defined as a flag), and stops, so --env-file .env.development.local will not be parsed.

I think you were right, just implementing a very simple arg parser that just looks for --env-file <value> [--env-file <value>] is the way to go. Let me put that together and add it to this PR. See e2618f8.

We should leave the --env-file flag definition in the cli.App so that it shows up in the help/usage text and parsed when urfave/cli parses the command line, but we won't actually do anything with the --env-file values it finds.

[dossy]

@hamza512b I also added tests to document the behavior of loadEnvFiles, in 29e3bcb. Please review. Thanks!

[hamza512b]

I have looked through your commits. As far as I can see, I do not see problems with the code.

[dossy]

@hamza512b I've updated the discussion #354, looking for any final feedback from users. If there's no issues raised, I think this is good to merge. Thanks!

[amacneil]

lgtm. I added a minor comment about the documentation but feel free to merge when ready

[Mizaro]

I have applied the suggestions, however I came up with an issue, if we pass multiple .env files to the function loadDotEnv (there is no way to do it right now) and some file does not exists how should we handle that?
For now, what I did is return if any file does not exist.

We can rely on godotenv's underlying behavior (see readFile), where it will return an error if a file cannot be opened, such as when it does not exist or when the process lacks sufficient permission to read the file.

Go's os.Open will return an error if the file does not exist, however, and as we want to not halt execution in this case, we need to check the error condition for os.ErrNotExist and ignore it if that's the case, otherwise process the error as normal. This way, if the .env file, or alternative file specified with --env-file, does not exist, we currently won't raise an error, and simply continue, which is equivalent to the file existing but being empty, anyway.

In my humble opinion, if a file does not exist, any tool should tell the user that the input is invalid.
Otherwise, a scenario where the user gives a path to a file without permission can look like everything is fine while the critical environment variable didn't get read.

[dossy]

In my humble opinion, if a file does not exist, any tool should tell the user that the input is invalid.
Otherwise, a scenario where the user gives a path to a file without permission can look like everything is fine while the critical environment variable didn't get read.

Hi, @Mizaro. Ordinarily, under normal circumstances, I absolutely agree with you.

However, environment files are a special case and I think ignoring files that do not exist (os.ErrNotExist), which is different than when the file exists but the process lacks the permission to read, is useful.

It's a common pattern to specify environment files as a list, such as []string{".env.local", ".env"}, where if the file .env.local exists, it is read first, and then .env is read, but only new keys in .env are incorporated into the environment, not overwriting anything set by .env.local. The desired behavior is, if .env.local exists, use it, but if it doesn't, it shouldn't raise an error. However, if there was any other error reading the file other than os.ErrNotExist, it will raise an error and let the user know.

This is currently the behavior implemented in dbmate, which I think is the right behavior. If you feel it's incorrect, please create an issue or discussion and we can determine if it needs to change.