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 support for specifying name/path of .env #941

Merged
merged 13 commits into from
Aug 9, 2021
Merged

Add support for specifying name/path of .env #941

merged 13 commits into from
Aug 9, 2021

Conversation

Celeo
Copy link
Contributor

@Celeo Celeo commented Aug 5, 2021

#802

This allows users to overwrite just looking for a file called .env in the working directory or its ancestors.

Open questions

  1. I have these two new args, --dotenv-filename and --dotenv-path mutually exclusive, with the latter taking precedence in the code over the former. Is that correct?
  2. Currently, if the path from --dotenv-path does not resolve to a file, then the existing functionality of searching for a file called ".env" in the working directory or its ancestors is taken. If the type of Config::dotenv_path was an Option<PathBuf>, I could differentiate between "user set" and "default value", but I didn't see any of the other args as options so I'm hesitant to introduce that without checking with you first. What's the desired behavior here?
  3. Do you want tests of the overridden dotenv file lookup?

This allows users to overwrite just looking for a file called .env in
the working directory or its ancestors.
@Celeo Celeo changed the title feat: Add support for specifying name/path of .env Add support for specifying name/path of .env Aug 5, 2021
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Thanks for this!

  1. I have these two new args, --dotenv-filename and --dotenv-path mutually exclusive, with the latter taking precedence in the code over the former. Is that correct?

What do you mean by the latter taking precedence? If they're mutually exclusive, isn't there no precedence either way, since they can't be passed together?

  1. Currently, if the path from --dotenv-path does not resolve to a file, then the existing functionality of searching for a file called ".env" in the working directory or its ancestors is taken. If the type of Config::dotenv_path was an Option<PathBuf>, I could differentiate between "user set" and "default value", but I didn't see any of the other args as options so I'm hesitant to introduce that without checking with you first. What's the desired behavior here?

Definitely make them Options. Although it's nice to avoid options where possible, for example by populating the config with default values, in this case we need to know what was passed, so it's fine. We should return an error if --dotenv-path is passed but we couldn't load an environment file from that path, so we'll need to know if it was passed. Also, both --dotenv-path and --dotenv-filename should silence the deprecation warning related to implicitly loading .env files, so they'll both need to be Options.

  1. Do you want tests of the overridden dotenv file lookup?

Yes please!

There should be tests that:

  • The options conflict
  • if --dotenv-path is passed and file doesn't exist, an error is printed
  • Passing --dotenv-path works and loads the right environment file
  • Passing --dotenv-filename works and loads the right environment file

Most of the integration tests use the test! macro. Recently, I improved the Test object, and started writing tests in an imperative style, which is a little less magical and more flexible, for example because you can check what's in the temporary directory afterwards, pass in a temporary directory, and do arbitrary setup. See dot_justfile_conflicts_with_justfile for an example of the imperative style. Both ultimately use the Test object, so they both test the same things, so feel free to use whichever you prefer.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/load_dotenv.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/load_dotenv.rs Outdated Show resolved Hide resolved
src/load_dotenv.rs Show resolved Hide resolved
@Celeo
Copy link
Contributor Author

Celeo commented Aug 6, 2021

My most recent battle with the CI job seems to be a success ;)

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Awesome, tests look great. Just a few minor comments.

src/config.rs Outdated Show resolved Hide resolved
src/load_dotenv.rs Outdated Show resolved Hide resolved
src/load_dotenv.rs Outdated Show resolved Hide resolved
tests/dotenv.rs Outdated Show resolved Hide resolved
tests/dotenv.rs Outdated Show resolved Hide resolved
tests/dotenv.rs Outdated Show resolved Hide resolved
Celeo and others added 2 commits August 7, 2021 19:31
Co-authored-by: Casey Rodarmor <casey@rodarmor.com>
@casey
Copy link
Owner

casey commented Aug 9, 2021

Let me know when this is ready for another look, I saw some pushes, but wasn't sure if it's ready for review.

@Celeo
Copy link
Contributor Author

Celeo commented Aug 9, 2021

Yup, ready for another review; I think I got all your feedback incorporated and working.

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

One last thing!

src/load_dotenv.rs Outdated Show resolved Hide resolved
@casey
Copy link
Owner

casey commented Aug 9, 2021

And def ping the PR with a comment when you're ready for me to take a look at it, I turned off push notifications because they get spammy for some of the repos I watch.

@Celeo
Copy link
Contributor Author

Celeo commented Aug 9, 2021

Fixed, and tests added.

Okay, last thing I swear

No worries!

push notifications because they get spammy

Yup, makes total sense!

@casey
Copy link
Owner

casey commented Aug 9, 2021

I just noticed the use of /bin/bash, can the recipes just be normal recipes, i.e. no shebang? I think on at least some systems, bash isn't installed as /bin/bash, so if it can't be removed, they should be #!/usr/bin/env bash.

@Celeo
Copy link
Contributor Author

Celeo commented Aug 9, 2021

Ah right, thanks for catching that. I was confused by the presence of output on stderr, but that's the printing of the recipe content. Swapped with the @ prefix for quieting that, and works. 👍

@casey casey merged commit e72e7dd into casey:master Aug 9, 2021
@casey
Copy link
Owner

casey commented Aug 9, 2021

Nice, merged! Thank you very much for the PR!

@Celeo Celeo deleted the 802-cli-flag-for-dotenv branch August 9, 2021 05:38
@xorander00
Copy link

Love the project, this and go-task are my go-to task runners currently :) I haven't had the chance yet to the read this entire PR yet, but I figured I'd throw my own $0.02 into it here.

The CLI args are nice and I'll likely use them, but personally I think since the potential name and number of .env files can vary between projects, the justfile itself should have a setting to specify the file(s) to load and the order in which to load them. go-task implements this approach by defining an array in Taskfile.yml that specifies the dotenv files to look for and load from left-to-right (later loaded dotenv files overriding values from previously loaded ones). That means the justfile would then kind of document the .env files as well, instead of having to figure it out via command line params.

@casey
Copy link
Owner

casey commented Aug 12, 2021

@xorander00 Definitely agree, this just happened to be implemented first. This has been discussed before, but I added a dedicated issue to track it in #945.

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.

3 participants