Skip to content
This repository has been archived by the owner on May 3, 2020. It is now read-only.

Refactor configuration handling in Config class. #515

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

alxbl
Copy link
Contributor

@alxbl alxbl commented Oct 7, 2018

This pull request adds a config.rb file which is responsible for centralizing the configuration loading and parsing across the project. It updates every occurrence of config_options lookups to use the Config class instead. It is built on top of #514 and so should be merged after it to avoid possible merge conflicts (although both should be fast-forwardable)

As a bonus, I've also made the lookup fallback to config.json.defaults when keys are not present in config.json. The goal is to make future additions transparent and not require manual intervention to enable those features. In theory, using this approach, config.json can contain only the overrides that are not default and possibly even make the configuration initialization step in first_time.rb obsolete.

This is the first step towards making it possible for containerized Serpico instances to load their configuration from a host-mounted volume.

@BuffaloWill
Copy link
Contributor

Rock on! I like this as a design change and definitely a positive move. I need to review the code further and do some more testing. Just wanted to let you know it is in the queue.

@alxbl
Copy link
Contributor Author

alxbl commented Oct 29, 2018

@MaxNad pointed out that Config didn't have a .each, which broke the configuration page. I just updated the branch to have that. (Also fixed merge conflicts)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants