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 ENV for all config options #181

Merged
merged 1 commit into from
Oct 16, 2016
Merged

Conversation

doomspork
Copy link
Member

Howdy @mmozuras! Following up on the conversation here, this PR enables support for ENV variables on all configuration options.

@@ -12,6 +12,7 @@ def initialize(config_hash = ConfigFile.new.to_h)
end

def consolidate_comments?
@config_hash['consolidate_comments'] ||= ENV['CONSOLIDATE_COMMENTS']
Copy link
Member Author

Choose a reason for hiding this comment

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

Should ENV variables take precedence?

Copy link
Member

Choose a reason for hiding this comment

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

@doomspork yes - they do everywhere else 😄

@mknapik
Copy link
Contributor

mknapik commented Oct 10, 2016

How about prefixing each ENV variable with PRONTO_ to avoid conflicts?

@doomspork
Copy link
Member Author

@mknapik the existing configuration variables don't rely on a prefix but I think that's a good idea, I'll leave that up to @mmozuras

@mmozuras
Copy link
Member

mmozuras commented Oct 14, 2016

@mknapik sounds like a good idea 👍. Could you (or @doomspork) make a separate pull request for it?

@doomspork
Copy link
Member Author

@mmozuras I can update this PR. Do you want me to prefix all the configuration options (a breaking change)?

@mmozuras
Copy link
Member

@doomspork nah, just update according to my one comment. And then let's make a separate PR with all options prefixed.

@doomspork
Copy link
Member Author

@mmozuras sounds good! 👍

@doomspork
Copy link
Member Author

doomspork commented Oct 16, 2016

Updated @mmozuras 👍

Once this is merged I can open a PR for the prefix additions (take a peek at the changes here).

@mmozuras
Copy link
Member

@doomspork thanks! 🙇

@mmozuras mmozuras merged commit 0ca8ed8 into prontolabs:master Oct 16, 2016
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.

None yet

3 participants