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 the get_config() method #16

Closed
wants to merge 1 commit into from
Closed

Add the get_config() method #16

wants to merge 1 commit into from

Conversation

tylrd
Copy link

@tylrd tylrd commented Jan 21, 2016

Will first check if the env variable exists, and if it doesn't, reads the JSON from a config file.

Will first check if the env variable exists, and if it doesn't, reads
the JSON from a config file.

Shorted the filename to config.json.  Required to be in the script
directory if no ENV is set.

json parsing error handling
@alex
Copy link
Owner

alex commented Jan 22, 2016

Going to need to think on this, I like the simplicity of only a single configuration method, and an env var is consistent with http://12factor.net/config. OTOH clearly a config file is useful for some folks, though I'm not sure requiring it to be in the same dir as the script is the best.

@stuart-lang
Copy link

@alex From my perspective as a newcomer, I was weirded out putting a json object into an environment variable, I actually put a path to a config file, which obviously didn't work, but this was my expectation of how it would/should work.

There is nothing inherently wrong with putting json into a env variable, other than a 2048 char limit, but I would bet that a lot of people would assume this would live in a file, and what to check it into source control.

In the article you reference I think they mean holding individual config values in env vars, not entire structures, which is very common.

By the way, thanks for this project, it's awesome!

@alex
Copy link
Owner

alex commented Mar 18, 2016

I'm continuing to think about a way to improve this; the really nice thing
about keeping it self-contained in an env var is it means you can deploy
with the existing Dockerfile on platforms that let you set env vars, but
not touch the file system (think: anything like heroku :-))

On Fri, Mar 18, 2016 at 10:25 AM, stuart-lang [email protected]
wrote:

@alex https://github.com/alex From my perspective as a newcomer, I was
weirded out putting a json object into an environment variable, I actually
put a path to a config file, which obviously didn't work, but this was my
expectation of how it would/should work.

There is nothing inherently wrong with putting json into a env variable,
other than a 2048 char limit, but I would bet that a lot of people would
assume this would live in a file, and what to check it into source control.

In the article you reference I think they mean holding individual config
values in env vars, not entire structures, which is very common.

By the way, thanks for this project, it's awesome!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#16 (comment)

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: D1B3 ADC0 E023 8CA6

@kageurufu
Copy link

I have an alternative fix for this in my branch, kageurufu/letsencrypt-aws@36a1d18. I haven't made a seperate pull request because it would include changes from my other request at #56

It still allows the current JSON is the environment variable, but also allows placing a file path there, or passing the --config command line argument, and finally if all else fails, defaults to config.json in the same directory as the script.

This should allow for far easier use, while still enabling more complicated deployments.

Personally, I run this script through cron, where passing a large json object in the environment is very messy, whereas now I can simply use

@daily /opt/letsencrypt-aws/venv/bin/python /opt/letsencrypt-aws/letsencrypt-aws.py update-certificates --config /opt/letsencrypt-aws/letsencrypt-aws.json

@tylrd tylrd closed this Aug 1, 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.

4 participants