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

fix: always respect file importance order #460

Merged
merged 2 commits into from
Jan 20, 2024
Merged

fix: always respect file importance order #460

merged 2 commits into from
Jan 20, 2024

Conversation

eriklovmo
Copy link
Contributor

@eriklovmo eriklovmo commented Aug 11, 2022

Solves the following issue:

Steps to reproduce

$ cat Gemfile
# frozen_string_literal: true

source "https://rubygems.org"

ruby File.read("#{File.dirname(__FILE__)}/.ruby-version")

gem "pry"

group :development, :test do
  gem "dotenv"
end
$ cat Gemfile.lock
GEM
  remote: https://rubygems.org/
  specs:
    coderay (1.1.3)
    dotenv (2.8.1)
    method_source (1.0.0)
    pry (0.13.1)
      coderay (~> 1.1)
      method_source (~> 1.0)

PLATFORMS
  ruby

DEPENDENCIES
  dotenv
  pry

RUBY VERSION
   ruby 3.1.2p20

BUNDLED WITH
   2.3.16

$ cat bin/scripts/console
#!/usr/bin/env ruby
# frozen_string_literal: true

require "pry"
Pry.start
$ cat .env.important
EXAMPLE_VAR=5678
$ cat .env.most.important
EXAMPLE_VAR=1234
$ bundle exec dotenv -o -f ".env.most.important,.env.important" ./bin/scripts/console
[1] pry(main)> ENV["EXAMPLE_VAR"]
=> "5678"
[2] pry(main)> exit
$ bundle exec dotenv -o -f ".env.important,.env.most.important" ./bin/scripts/console
[1] pry(main)> ENV["EXAMPLE_VAR"]
=> "1234"

Expected behavior

The environment variables in .env.most.important take precedence over those in .env.important.

$ bundle exec dotenv -o -f ".env.most.important,.env.important" ./bin/scripts/console
[1] pry(main)> ENV["EXAMPLE_VAR"]
=> "1234"

Quoting from the README.md file:

The dotenv executable also accepts a single flag, -f. Its value should be a comma-separated list of configuration files, in the order of most important to least. All of the files must exist. There must be a space between the flag and its value.

$ dotenv -f ".env.local,.env" ./script.rb

Actual behavior

The environment variables in .env.most.important are overloaded by those in .env.important, i.e. the file priority policy is violated.

$ bundle exec dotenv -o -f ".env.most.important,.env.important" ./bin/scripts/console
[1] pry(main)> ENV["EXAMPLE_VAR"]
=> "5678"

System configuration

dotenv version: 2.8.1

Ruby version: 3.1.2

Additional info

As thoroughly described in issue #424, the overload option violates what seems to be the official, documented file priority policy. A patch for Rails was offered in PR #453, while these changes attempt to target the root cause, including updating the behavior of the CLI.

Some notes:

  • This is a breaking change.
  • The documentation and some of the tests appear to contradict each other. It is possible that me (and other affected parties) have misunderstood how the overload option is meant to function. Hence, an alternative solution is to update the documentation where applicable so that it is less ambiguous. Specifically, it would be good to address how the -f and -o flags work in tandem.
  • Suggestions for changes/other solutions are very welcome.

@swalke16
Copy link

Would love to see this get merged soon as we spent several hours the last couple days trying to figure out why things weren't loading as we expected with the overload flag on.

@amitsaxena
Copy link

@bkeepers it can be really helpful to clarify the intended behaviour.🙏 Is this a bug, or is this the way we should expect things to work: with override option the env variable in last file takes precedence, compared to a normal scenario where the first file takes precendence.

@luizkowalski
Copy link

would like to see this merged too, seeing the same issue locally: I have two files .env and .env.development.local and I'm getting the .env values loaded locally (they are intended to be used in production).

took me a couple of hours to find this 😞

…ty-policy-when-overloading

* origin/master:
  Tweak logic for deciding which method to load
  Bump actions/checkout from 3 to 4
  Remove EOL Ruby versions from build matrix (2.5, 2.6, 2.7)
  Fix lint errors
  Add Ruby 3.3 to the build matrix
  Appease Standard.rb
  Optionally ignore missing files from CLI
  Use console syntax highlights for console commands
@bkeepers bkeepers merged commit 32c521b into bkeepers:master Jan 20, 2024
4 checks passed
@bkeepers
Copy link
Owner

@eriklovmo thanks for the pull request, and sorry it took so long to review and merge.

I'm going work on a few other breaking changes and will release this in 3.0 soon.

indiebrain referenced this pull request in powerhome/power-web-development-interview Feb 13, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [dotenv-rails](https://togithub.com/bkeepers/dotenv) | `2.8.1` ->
`3.0.0` |
[![age](https://developer.mend.io/api/mc/badges/age/rubygems/dotenv-rails/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/rubygems/dotenv-rails/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/rubygems/dotenv-rails/2.8.1/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/rubygems/dotenv-rails/2.8.1/3.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>bkeepers/dotenv (dotenv-rails)</summary>

###
[`v3.0.0`](https://togithub.com/bkeepers/dotenv/blob/HEAD/Changelog.md#300)

[Compare
Source](https://togithub.com/bkeepers/dotenv/compare/v2.8.1...v3.0.0)

**Breaking Changes**

- Ruby >= 3.0 and Rails >= 6.1 are now required. Lock dotenv to `~> 2.0`
if you are using an outdated Ruby or Rails version.
[https://github.com/bkeepers/dotenv/pull/466](https://togithub.com/bkeepers/dotenv/pull/466),
[https://github.com/bkeepers/dotenv/pull/471](https://togithub.com/bkeepers/dotenv/pull/471)
- `\n` is no longer expanded into a newline in quoted strings. Use
multi-line strings with real line breaks, or set
`DOTENV_LINEBREAK_MODE=legacy` to preserve the old behavior.
[@&#8203;nitsujri](https://togithub.com/nitsujri)
[https://github.com/bkeepers/dotenv/pull/423](https://togithub.com/bkeepers/dotenv/pull/423)
- `ENV` will be [automatically restored between
tests](https://togithub.com/bkeepers/dotenv#autorestore-in-tests)
(`ActiveSupport::TestCase` and `Rspec`).
[https://github.com/bkeepers/dotenv/pull/472](https://togithub.com/bkeepers/dotenv/pull/472),
[https://github.com/bkeepers/dotenv/pull/475](https://togithub.com/bkeepers/dotenv/pull/475)
- Fixed precedence when using `Dotenv::Rails.overload`. So now
`.env.development.local` will overwrite `.env.local`, which will
overwrite `.env.development`, which will overwrite `.env`.
[@&#8203;eriklovmo](https://togithub.com/eriklovmo) -
[https://github.com/bkeepers/dotenv/pull/460](https://togithub.com/bkeepers/dotenv/pull/460)
- The instrumentation event `dotenv.load` has been renamed to
`load.dotenv` to properly make use of namespaces in
[ActiveSupport::Notifications](https://guides.rubyonrails.org/active_support_instrumentation.html)
[https://github.com/bkeepers/dotenv/pull/472](https://togithub.com/bkeepers/dotenv/pull/472)

**Other improvements**

- All changes to ENV will be logged in Rails apps.
[https://github.com/bkeepers/dotenv/pull/473](https://togithub.com/bkeepers/dotenv/pull/473)
- Fixed an issue where `rake` loaded development files
(`.env*development`) for test-related tasks.
[https://github.com/bkeepers/dotenv/pull/470](https://togithub.com/bkeepers/dotenv/pull/470)
- Add `-i`/`--ignore` option to `dotenv` CLI to optionally ignore
missing files. [@&#8203;stevenharman](https://togithub.com/stevenharman)
[https://github.com/bkeepers/dotenv/pull/463](https://togithub.com/bkeepers/dotenv/pull/463)
- You can [customize which files get
loaded](https://togithub.com/bkeepers/dotenv#customizing-rails) by
setting `Dotenv::Rails.files`.
[https://github.com/bkeepers/dotenv/pull/468](https://togithub.com/bkeepers/dotenv/pull/468)

**Deprecations**

- The `dotenv-rails` gem is now superfluous. It's not technically
deprecated yet and will continue to work, but the `dotenv` gem does the
same thing.
[https://github.com/bkeepers/dotenv/pull/468](https://togithub.com/bkeepers/dotenv/pull/468)
- `Dotenv::Railtie` has been deprecated. Use `Dotenv::Rails`.
[https://github.com/bkeepers/dotenv/pull/468](https://togithub.com/bkeepers/dotenv/pull/468)
- `Dotenv.overload` has been replaced with `overwrite`. `overload` will
still work and is not technically deprecated, but documentation refers
to `Dotenv.overwrite` now.
[https://github.com/bkeepers/dotenv/pull/469](https://togithub.com/bkeepers/dotenv/pull/469)

**New Contributors**

- [@&#8203;stevenharman](https://togithub.com/stevenharman) made their
first contribution in
[https://github.com/bkeepers/dotenv/pull/463](https://togithub.com/bkeepers/dotenv/pull/463)
- [@&#8203;eriklovmo](https://togithub.com/eriklovmo) made their first
contribution in
[https://github.com/bkeepers/dotenv/pull/460](https://togithub.com/bkeepers/dotenv/pull/460)
- [@&#8203;nitsujri](https://togithub.com/nitsujri) made their first
contribution in
[https://github.com/bkeepers/dotenv/pull/423](https://togithub.com/bkeepers/dotenv/pull/423)

**Full Changelog**:
bkeepers/dotenv@v2.8.1...v3.0.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/powerhome/power-web-development-interview).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

5 participants