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

Use Config gem to read and validate configuration settings #2170

Closed
wants to merge 1 commit into from

Conversation

gravitystorm
Copy link
Collaborator

This is a partial solution to #2169 using the Config gem. It features:

  • validating particular settings, and what format those settings should be, at boot time
  • supports our existing config file structure (through some hackery), including special settings for tests
  • works with existing application.yml files
  • supports environment variable overrides (but see below)

However:

  • It needs to work in two stages, using initializers/settings.rb to revisit the setup with our custom application.yml file. This isn't ideal, since running everything in initializers/config.rb would mean settings are processed at the before_configuration stage, whereas settings.rb runs later.
  • It still requires our preinitializer, in order that the constants required in application.rb are available. application.rb runs before the code in initializers/settings.rb
  • Although it supports environment variables, it can't support the same naming convention as we currently use. For example, we currently support e.g. 'OSM_API_TIMEOUT=foo'. If we set the prefix to 'OSM' and the env_separator to '_', then that parses to 'Settings.api.timeout' instead of 'Settings.api_timeout'. I'm really not sure if this is a big deal or not.
  • There's a corner case involving environment variables that look-like-numbers-but-aren't. For example, with env OSM.api_version=0.7 bundle exec ..., the 0.7 gets converted to a float and then fails the string validation.

If we can ignore environment variables (can we?) then this still is an improvement on what we have already, due to the validations. However, it could be even better if there are other refactorings that can remove the need for the preinitializer code entirely, and I welcome any feedback.

@tomhughes
Copy link
Member

Having done some testing I think most of the above problems go away if we just bite the bullet and move our settings to settings.yml with per-environment overrides as necessary.

Doing that, and removing the preinitiailiser and your settings.rb initialiser and with various extra conversions to use Settings. it is possible to get to the console and the rest is just a question of more work really.

The only problem (given that the config.rb initialise is deliberately loaded very early) is the very first use of STATUS in config/application.rb which occurs before it is loaded. Maybe we should just say that status is always set from the environment and not use settings for it?

As far as I general environment support is concerned I doubt it is ever used. I had forgotten we supported it myself! So yes we can likely get rid of it.

@gravitystorm
Copy link
Collaborator Author

Having done some testing I think most of the above problems go away if we just bite the bullet and move our settings to settings.yml with per-environment overrides as necessary.

I was mulling this over last night, before I read your message, and I came to a similar conclusion. I realised that it's the only sensible approach to using settings in other initializers, which is exactly why the gem loads values so early. With this PR's approach of loading settings in a regular initializer, it would be hard to get it working at all.

The only problem (given that the config.rb initialise is deliberately loaded very early) is the very first use of STATUS in config/application.rb which occurs before it is loaded. Maybe we should just say that status is always set from the environment and not use settings for it?

If that's workable with you, then great. I had a brief look around to see if there's anyone else with a "database offline" mode in rails, and how they manage it, but my googling was just picking up unrelated scenarios.

So yes we can likely get rid of it.

Great. I'd like to support environment variables since it's useful when e.g. deploying to Heroku, but it's much easier if we can break compatibility with the existing ones if they are unused. I had no idea that we supported them either, until yesterday.

I'll close this PR, and work on the "bite the bullet" approach as a replacement.

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.

None yet

2 participants