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 load to respect existing environment variables over env file when doing variable interpolation #323

Merged
merged 3 commits into from
Apr 18, 2018

Conversation

cbjoseph
Copy link
Contributor

@cbjoseph cbjoseph commented Feb 23, 2018

Closes issue #319.

@cadwallion
Copy link
Collaborator

👋 @cbjoseph !

Apologies that this has been sitting for so long, I'm part of a trio of new maintainers of this project, and we're trying to clear out the backlog of folks waiting for responses.

There appears to be a merge conflict associated with some of our work clearing up said backlog. Could you please resolve the conflict so I can review?

Thank you!

@cbjoseph
Copy link
Contributor Author

@cadwallion resolved conflicts! Thanks for taking a look.

@jonmagic
Copy link
Collaborator

@cbjoseph it looks like there is one rubocop error:

lib/dotenv/substitutions/variable.rb:44:1: C: Trailing whitespace detected.

I believe fixing that up should be the last step.

@cbjoseph
Copy link
Contributor Author

@jonmagic oops! fixed rubocop error, should be all good now

@cadwallion
Copy link
Collaborator

Looks good, thanks for the contribution!

@cadwallion cadwallion merged commit ae95d20 into bkeepers:master Apr 18, 2018
@nijikon
Copy link

nijikon commented Apr 26, 2018

I think that something is still off with the implementation.

balrog:~ n$ cat .env
TEST=5
TEST_COMBINED=combined_${TEST}
balrog:~ n$ TEST=10 ruby dotenv.rb
Before loading .env...

TEST=10
TEST_COMBINED=

{"TEST"=>"5", "TEST_COMBINED"=>"combined_10"}
After loading .env...

TEST=5
TEST_COMBINED=combined_10
#!/usr/bin/env ruby
# frozen_string_literal: true

def display_envs()
  puts
  puts "TEST=#{ENV['TEST']}"
  puts "TEST_COMBINED=#{ENV['TEST_COMBINED']}"
  puts
end

puts "Before loading .env..."
display_envs

require 'dotenv'
puts Dotenv::Environment.new('.env', true).inspect
ENV.update(Dotenv::Environment.new('.env', true))

puts "After loading .env..."
display_envs

@nijikon
Copy link

nijikon commented Apr 26, 2018

/cc @cbjoseph @cadwallion

@cbjoseph
Copy link
Contributor Author

@cadwallion Can you comment on expectation of this Environment class? My assumption was that it's an internal tool, since it seems undocumented. Could you provide more context on its use case?

@jessedoyle
Copy link
Contributor

jessedoyle commented May 1, 2018

This change just caught us off guard as well with a gem update.

@cbjoseph: Is it documented anywhere that Dotenv::Environment is internal and subject to breaking changes?

Ideally, any changes to a public method signature should be backwards compatible.

What does everyone think about making the is_load parameter here default to true?

i.e.

def initialize(filename, is_load = true)
  @filename = filename
  load(is_load)
end

jessedoyle added a commit to jessedoyle/dotenv that referenced this pull request May 1, 2018
* Commit 7f82f73 introduced a backwards-incompatible change with
  the current API with respect to `Dotenv::Environment#intialize`
  by changing the method signature.
* Make the new parameter default to false to retain backwards
  compatibility.
* Similar changes were made for `Dotenv::Parser` in bkeepers#338.

SEE: bkeepers#323
SEE: bkeepers#338
KrauseFx added a commit to fastlane/ci that referenced this pull request May 3, 2018
Breaking change was introduced in minor version with bkeepers/dotenv#323
@KrauseFx
Copy link

KrauseFx commented May 3, 2018

I also wanted to jump in that this broke many setups for fastlane and fastlane.ci, as it was introduced with a minor version bump. Adding a default parameter with #345 sounds like a great fix 👍

@EvanHahn
Copy link

EvanHahn commented May 3, 2018 via email

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.

8 participants