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

[dotenv-rails] .env.local should be loaded in test environments #418

Closed
erwanst opened this issue Oct 26, 2020 · 20 comments
Closed

[dotenv-rails] .env.local should be loaded in test environments #418

erwanst opened this issue Oct 26, 2020 · 20 comments

Comments

@erwanst
Copy link

erwanst commented Oct 26, 2020

Hi (and since it is my first contribution to the project: thank you maintainers for Dotenv !)

#280 removed .env.local from the files loaded by Dotenv when Rails is loaded in the test environment.

As discussed in the last comments on the PR, that behavior can be unexpected and is too restrictive for some legitimate (common ?) setups. Having to duplicate variables from .env.local to .env.test.local on the grounds that .env.local should not be used in test environments doesn't really make sense in my opinion. Also, it is up to developers to make sure they configure their CI correctly, I don't think Dotenv should or even can strictly enforce what files are used for what. The documentation is already very clear about what should be ignored from git, and why, and maybe it is enough ?

I created at PR implementing the change: #417

I understand that changing the behavior of Dotenv on that point is tricky because it would probably break things in some projects that rely on .env.local not being loaded for tests, and I hope there is a way forward nonetheless.

Thanks for your time :)

Dotenv: v2.7.6

@fschwahn
Copy link

Just stopping by to second this. It's counter-intuitive that there's such a special case for .env.local. In my case, database / redis ports live in .env.local, which applies to all (ie. development & test) environments. It means I have to copy everything over to .env.test.local and always remember to make changes in both files.

@erwanst
Copy link
Author

erwanst commented Dec 28, 2020

Maintainers, I would be grateful for your feedback on this issue :)

If the change of behavior introduced by the attached PR is a blocker, and turning it into a conditional, opt-in feature would help, I would gladly amend the PR.

@codedeleter
Copy link

Any chance this could get in? I also find the current behavior very unintuitive.

@erwanst
Copy link
Author

erwanst commented Apr 20, 2021

Hello maintainers, friendly ping in the hope of getting your feedback on the issue :)

@erwanst
Copy link
Author

erwanst commented Jun 1, 2021

Hi maintainers, is there a reason why, several months later, this issue still has no feed-back from you ? Again, I would understand if the issue is not a priority or not even a desired change. If that is the case, I'll simply maintain my own fork and be done with it. If not, is there a way forward ?

@jjercx
Copy link

jjercx commented Jun 14, 2021

I just had same error. Expecting .env.local to be used while running tests, but surprised when it was actually reading .env

I was going to say "docs should be updated" yet I just noticed:

Local overrides. This file is loaded for all environments except test.

Yet, I was confused by the Wherever the file is under environment. Probably the except test should be there as well

@codyrobbins
Copy link

Yup, I’m in the same boat—this behavior makes no sense, breaks expectations, and makes you manually keep values in sync across multiple files for no discernible benefit or reason.

@ktimothy
Copy link

ktimothy commented Dec 30, 2021

Just to contribute to the issue and keep it alive: this is extremely counter-intuituve behaviour with no good rationale behind it. Let's just fix it, it does not sound like a hard think to improve. You even have an existing PR! =)

@fschwahn
Copy link

Not to pile on, but even the original author admits that this quite an opinionated behavior, and would be fine with reverting it: #280 (comment)

@dgm
Copy link

dgm commented Mar 27, 2023

I too just got bit by this... I have my local database credentials in .env.local and was quite surprised that my tests would not work.

@bkeepers
Copy link
Owner

In dotenv 3.0, you can customize the files you want loaded. See Customizing Rails. I'm hoping to have that release out soon.

Until then, you can always add Dotenv.overload ".env.local" in config/environments/test.rb or your test helper.

cc #468

@codyrobbins
Copy link

While it’s great that 3.0 will allow overriding this behavior, I still respectfully think this change should be reverted completely. The problem here is really that the default behavior makes no sense for most people. Instead of requiring custom configuration straight out of the box to undo a poorly considered change, I think it’s preferable to have it work by default in a way that doesn’t break expectations and then allow those with the non-majority use cases to make use of overrides to customize the behavior to suit their needs. As it is we’re basically left with an override to override a hard-coded override that shouldn’t even exist in the first place.

@bkeepers
Copy link
Owner

@codyrobbins I hear you and understand where you're coming from. Unfortunately, reverting it will likely cause harm to people that rely on the current behavior, and I'm not willing to risk it. Worst case scenario, imagine someone has DATABASE_URL=… set to their production database in .env.local (I know, terrible idea), and then runs rails test. They've done this a million times in the last 5 years to test production data in development and it's fine (they tell themself), but someone on their team recently upgraded dotenv and now their production database is gone. dotenv is too widely adopted to risk this change.

If consistency is what you want, I would sooner drop support for .env*local entirely than revert the change. Variables not getting set will cause less harm than varibales getting set to unexpected values.

It's a one line change to your app for anyone that disagrees.

@byronbowerman
Copy link

IMO it's worth noting that rails has had an environment check designed to prevent just that sort of eventuality.

@bkeepers
Copy link
Owner

IMO it's worth noting that rails has had an environment check designed to prevent just that sort of eventuality.

It checks that !Rails.env.production?, but that’s not the issue here.

@fschwahn
Copy link

No, it's more nifty than that. It checks in which environment the database was created (this is stored in a metadata table in the DB itself), and prevents dropping it in case it is a production DB. See rails/rails#22967

@bkeepers
Copy link
Owner

@fschwahn nice, I’d did realize that.

Still, it only protects your database. People could have credentials to other production services, so my reasoning is the same. I’m not willing to risk it.

@fschwahn
Copy link

FWIW I think this is very sound reasoning. It's unfortunate that this was introduced in the first place, but I understand that it is hard to change now.

@byronbowerman
Copy link

I'm not thrilled with it, but I understand, especially since it seems easy enough to deal with.

@dgm

This comment was marked as off-topic.

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

9 participants