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

Support loading multiple dotenvs #7943

Merged
merged 3 commits into from
Jan 24, 2017
Merged

Conversation

lacostej
Copy link
Collaborator

@lacostej lacostej commented Jan 21, 2017

Things I am a bit unsure of:

  • do we want to rename --env into --envs
  • Actions.lane_context[Actions::SharedValues::ENVIRONMENT] should a new one be added with an array? should the original one keep the CSV passed on the CL?
  • documentation

@@ -90,6 +90,9 @@ lane :deploy_all do
end
```

And you can combine multiple envs in one go
Ex: `fastlane build --env app1,env1,env2` will use `.env.app1` `.env.env1` and `.env.env2`
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those file look awfully similar. Are they meant to be the same? Why not sync them automatically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will prepare a patch once this one is accepted.

env_file = File.join(base_path, ".env.#{env}")
UI.success "Loading from '#{env_file}'"
Dotenv.overload(env_file)
if envs
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use return if envs.nil?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored

Copy link
Collaborator

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

Looking good, couple of general comments, but the api itself seems 👍

if env
return unless envs

envs = envs.split(",") # multiple envs?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe call the arg "env_descriptor" or similar? Overwriting the var makes this harder to scan read

# Loads .env file for the environment(s) passed in through options
envs = [envs] unless envs.kind_of? Array

envs.each do |env|
env_file = File.join(base_path, ".env.#{env}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd break this out into a load_env(env) fn for readability, because right now this fn is getting pretty complex

expect(ENV['DOTENV']).to eq(expected_value)
ff = Fastlane::LaneManager.load_dot_env(envs)
ENV.each do |k, v|
puts "#{k} = #{v}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be removed before merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doh!

* documented
* extracted a method
* removed unecessary Array boxing
* renamed original argument for clarity
* ensured lane_context contains a String (the original argument)
@lacostej
Copy link
Collaborator Author

@dantoml comments tentatively addressed in 08e0df7

Copy link
Collaborator

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

LGTM

@lacostej lacostej merged commit 9125989 into fastlane:master Jan 24, 2017
@KrauseFx KrauseFx mentioned this pull request Jan 24, 2017
@fastlane fastlane locked and limited conversation to collaborators Apr 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants