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

Add required_keys!(...) method to Env #75

Closed
wants to merge 1 commit into from

Conversation

mulder
Copy link

@mulder mulder commented Oct 30, 2013

Allows you to do an aggressive post launch test to confirm all the
keys your apps need are present and ready to go.

It will raise an exception if any are missing and it will tell you
which of the required keys are missing.

Use this in rake tasks so cap can bail mid deploy, or in production.rb
to stop server boots.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6d71e3c on mulder:feature/required_keys into 3afe94b on laserlemon:master.

@mulder
Copy link
Author

mulder commented Oct 30, 2013

@laserlemon StringInquirer is being used in the application_spec. This is my super cheap fix so I could start with green tests.... master was red.

I'm not sure how you want to fix the failing test; likely the simplest fix is to include the StringInquirer in the spec.

@laserlemon
Copy link
Owner

Yeah, I'd recommend putting that require line in the file where the inquirer is used.

@mulder
Copy link
Author

mulder commented Oct 30, 2013

@laserlemon I've fixed up the broken spec on #76 ; I've rebased this one off the fixed spec_helper.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5567298 on mulder:feature/required_keys into 3afe94b on laserlemon:master.

@mulder
Copy link
Author

mulder commented Nov 1, 2013

@laserlemon I'm liking a lot of the work done on master. When are we going to see it all in a release? And could this feature make it in?

@laserlemon
Copy link
Owner

@mulder Thanks! Yes, master is shaping up but I've been so swamped with work work that it hasn't moved in a little bit. I like this idea quite a bit and I think it should be included in the next release. I may decide to change the naming to be a little more verb-y like Figaro.require or Figaro.require_keys.

@mulder
Copy link
Author

mulder commented Nov 1, 2013

Moving it up to Figaro namespace makes sense to me. I put on env because that is where all the env key/var magic was. I'm not fussy on the name, nor the location, but if the purpose is raising an error I'd like to see the ! stay.

I'm up for pitching in on this project if you need a hand with other features/refactors.

@arthurnn
Copy link

arthurnn commented Nov 1, 2013

Tests run just fine in here without 2097116 .

@mulder
Copy link
Author

mulder commented Nov 1, 2013

not for me using rake spec

This allows you to do an aggressive post launch test to confirm all the
keys your apps need are present and ready to go.

It will raise an exception if any are missing and it will tell you
which of the required keys are missing.

Use this in rake tasks so cap can bail mid deploy, or in production.rb
to stop server boots.
@mulder
Copy link
Author

mulder commented Nov 1, 2013

And there is the problem; bundle exec does a much better job. I've rebased away the bad.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8483f97 on mulder:feature/required_keys into 3afe94b on laserlemon:master.

@EiNSTeiN-
Copy link

@laserlemon is there something still holding this or could we get it merged?

@laserlemon
Copy link
Owner

This PR won't be merged in as-is but the idea is great. I'm of the opinion that you shouldn't write a bang method if there's no non-bang counterpart. I think the winner is Figaro.require_keys that can accept an array of keys or multiple keys as arguments.

In the case where values for provided keys are not set, an error will be raised with messaging that includes every key that's missing.

I'll see what we can do for release with version 1.0. Thank you!

@laserlemon laserlemon added this to the v1.0 milestone Apr 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants