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

Selective env #160

Merged
merged 1 commit into from
Jan 20, 2015
Merged

Selective env #160

merged 1 commit into from
Jan 20, 2015

Conversation

glenngillen
Copy link
Contributor

This pull request allows YAML and environment variable configs to co-exist with each other. This also means a user can specify an environment variable of NESTA* without causing nesta to completely change where it reads it's config from.

I also had to change the way fallback to YAML happens as falsy values defined in ENV would be ignored.

@gma
Copy link
Owner

gma commented Jan 19, 2015

Did you update your master branch before you forked it? It's not applying cleanly…

@glenngillen
Copy link
Contributor Author

Yeah, but I did a selective patch to exclude a bunch of white space changes and I see now the PR says there are conflicts. Let me take another look later this evening.

On Mon, Jan 19, 2015 at 6:16 PM, Graham Ashton notifications@github.com
wrote:

Did you update your master branch before you forked it? It's not applying cleanly…

Reply to this email directly or view it on GitHub:
#160 (comment)

@glenngillen glenngillen force-pushed the selective-env branch 2 times, most recently from 97f1162 to eefcf48 Compare January 19, 2015 08:49
@glenngillen
Copy link
Contributor Author

Fixed, must've had an old fork and didn't realize it.

What do you think?

@@ -91,7 +91,7 @@ def self.yaml_exists?
private_class_method :yaml_exists?

def self.can_use_yaml?
ENV.keys.grep(/^NESTA/).empty? && yaml_exists?
yaml_exists?
Copy link
Owner

Choose a reason for hiding this comment

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

We no longer need both yaml_exists? and can_use_yaml?; they're identical. I'll take a look at nailing one of them.

@gma gma merged commit eefcf48 into gma:master Jan 20, 2015
@gma
Copy link
Owner

gma commented Jan 20, 2015

Sorted. I wrote up a big explanatory comment in the merge commit.

@glenngillen
Copy link
Contributor Author

Woo! Thanks.

If/when you get a chance could I get a version bump so I can lock this change as a dependency on a few things?

@gma
Copy link
Owner

gma commented Jan 23, 2015

Will do. I'd like to review a PR that fixes plugin generation and get that into the same release. Are you waiting on it to get other work done?

@glenngillen
Copy link
Contributor Author

Nothing pressing, I've got Gemfiles pulling things via git in the interim.

On Fri, Jan 23, 2015 at 5:46 PM, Graham Ashton notifications@github.com
wrote:

Will do. I'd like to review a PR that fixes plugin generation and get that into the same release. Are you waiting on it to get other work done?

Reply to this email directly or view it on GitHub:
#160 (comment)

@gma
Copy link
Owner

gma commented Jan 23, 2015

Okay. I'll see if I get any spare time on Sunday. If it's not done within the week, feel free to remind me.

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.

2 participants