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

Flatten config variables in yml files #296

Merged
merged 3 commits into from
Mar 22, 2015

Conversation

maureendugan
Copy link
Contributor

Flattening the variables in the yml files constitutes a breaking change. But I wanted to ask - should the variables stored in themes/*/settings.yml be moved to config/secrets.yml?

See the previous discussions for pull request #274 and the original issue #158

@reidab
Copy link
Member

reidab commented Jan 26, 2015

The stuff in themes/*/settings.yml should be specific to the theme itself. We'd originally thought that this would include things like the site name and the timezone, but I think most of these make more sense moving into the deployment-specific config directory.

The only thing I think it makes sense to leave in the theme settings file is the precompile_assets key. This value shouldn't need to be customized outside of version control, so it will be save to use the existing SettingsReader class to parse it.

@maureendugan
Copy link
Contributor Author

In line with the discussion of turning Calagator into an engine... Let's merge the current PR and the next steps will be to:

  1. Add figaro to call secrets and settings from ENV variables instead of settings_reader.rb and secrets_reader.rb
  2. Create an initializer in the 'config' directory of the enclosing app (i.e. "some-calagator.org").

@botandrose
Copy link
Contributor

👍 I'd like to see this merged so we can replace the secrets system with Figaro in the engine.

@reidab
Copy link
Member

reidab commented Mar 4, 2015

@botandrose do you think we should work on merging this now and rebase the engine work on top of it, or work on merging the engine work and then rethink settings in that context?

@botandrose
Copy link
Contributor

@reidab I just spoke to Maureen, and I'm leaning towards merging the engine work first, and then she and I can revisit this afterwards.

@maureendugan
Copy link
Contributor Author

Rebased these changes.

@botandrose
Copy link
Contributor

👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 67a748d on moedu23:flatten_config_variables into 7a25f19 on calagator:master.

reidab added a commit that referenced this pull request Mar 22, 2015
Flatten config variables in yml files
@reidab reidab merged commit 2aee30d into calagator:master Mar 22, 2015
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