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.

Sign up for GitHub

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

Jump to bottom

Add support for environment variables as config #61

Merged
merged 3 commits into from Jun 21, 2016
Merged

Add support for environment variables as config #61

merged 3 commits into from Jun 21, 2016

Conversation

andygrunwald
Copy link
Contributor

In terms of the 12 factor apps configuration settings should be applied as env vars as well.
See http://12factor.net/config

This pull request enables the configuration for mlstats via env vars.
All other configuration types (incl. default values) are still supported.

This env vars are supported:

  • MLSTATS_DB_DRIVER
  • MLSTATS_DB_USERNAME
  • MLSTATS_DB_PASSWORD
  • MLSTATS_DB_HOSTNAME
  • MLSTATS_DB_NAME
  • MLSTATS_WEB_USERNAME
  • MLSTATS_WEB_PASSWORD
  • MLSTATS_COMPRESSED_DIR
  • MLSTATS_REPORT_FILENAME
  • MLSTATS_REPORT
  • MLSTATS_QUIET
  • MLSTATS_FORCE

The names of the env vars follows the schema APPNAME_SECTION_CONFIG.

With this PR the usage of mlstats for Docker or "cloud" envs is much easier.

See 12 Fractured Apps for further read.

See chaoss/grimoirelab-sortinghat#45 for the sortinghat edition.

@andygrunwald
Copy link
Contributor Author

Any update on this?

@andygrunwald
Copy link
Contributor Author

@gpoo @sduenas @jgbarah Any update?

@gpoo
Copy link
Member

gpoo commented Jun 17, 2016 •
edited

Hi Andy,

Te patch needs to be updated as mlstats is using argparse now.

I think it could be something like:

    db_driver = opts.db_driver or os.getenv('MLSTATS_DB_DRIVER', 'mysql')

What do you think?

@andygrunwald
Merge remote-tracking branch 'upstream/master' into env-config
94dfbb5 
* upstream/master:
  Add mailing_list_url column to PSQL schema
  Use argparse to replace the old getopt
  analyzer: Check if year is higher than 1900. Fixes 63
  Prepare regex to clean up the Reply-To field
  Refactor analyzer to split mbox from message parser
  test: Assume DB driver as sqlite is none is specified
  Add offset option to start at an arbitrary message. Fixes #62
  backends: Add Webdirectory backend. Fix #35
  Move fetch_remote_resource to pymlstats/htmlparser
  Decouple the archive retrieval process from the application
@andygrunwald
Copy link
Contributor Author

@gpoo I updated this PR.
I choosed the default keyword over your approach, because i think that argument variables should overwrite env vars. Env vars are good to set them on the system / container. But once you want to overwrite those, you can use the command line args.

Anything missing that avoids a merge?

@gpoo
Copy link
Member

gpoo commented Jun 20, 2016

I think both approaches give the same result. The right part of the or is only evaluated if the left part returns None or False.

Anyhow, I agree that the default is a cleaner approach :-)

@gpoo
Copy link
Member

gpoo commented Jun 20, 2016

I forgot to say, it looks ok to me.

@andygrunwald
Copy link
Contributor Author

That is great and i learned a new thing (with the or). Thank you!

@gpoo Are you fine with it when i merge it?

@gpoo gpoo merged commit 1f727f1 into MetricsGrimoire:master Jun 21, 2016
@gpoo
Copy link
Member

gpoo commented Jun 21, 2016

Totally.

@andygrunwald andygrunwald deleted the env-config branch June 22, 2016 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andygrunwald @gpoo