search

magfest / sideboard

BSD 3-Clause "New" or "Revised" License
6 stars 13 forks source link

Add a SIDEBOARD_CONFIG_OVERRIDES environment variable to specify config file paths #66

Closed RobRuana closed 7 years ago

RobRuana commented 7 years ago

I believe @EliAndrewC has done some work on this already in an upstream (downstream?) fork. I don't know how the current implementation works, so this is a proposal of how I think it should work. As always, I welcome comments and suggestions!

Proposal

Upon startup, sideboard will read an environment variable SIDEBOARD_CONFIG_OVERRIDES to determine where it should look for custom config files. SIDEBOARD_CONFIG_OVERRIDES will contain a semicolon separated list of absolute and relative paths to config files.

SIDEBOARD_CONFIG_OVERRIDES="/absolute/path/config.ini;relative/path/config.ini"

Implementation

In all cases, sideboard will first look for config files in the following locations, in order:

If SIDEBOARD_CONFIG_OVERRIDES is empty or does not exist in the environment, then no further configuration is performed.

Otherwise, sideboard will look for each config file specified by SIDEBOARD_CONFIG_OVERRIDES. If an absolute path is used, sideboard will only load the specified config file once. If a relative path is used, then sideboard will search for the config file relative to the sideboard directory, and again relative to each plugin directory.

If any config files are specified with a <FILENAME>-defaults.<EXT> suffix, then sideboard will also look for a similarly named file without the -defaults suffix, <FILENAME>.<EXT>. So development-defaults.ini will also attempt to load development.ini, and test-defaults.ini will also attempt to load test.ini.

In no case is a missing config file considered an error. If any of the specified config files do not exist, sideboard should issue a WARNING (maybe an INFO?) and continue without error.

Examples

Ex: SIDEBOARD_CONFIG_OVERRIDES not specified in environment

Running sideboard with uber and reports plugins, and SIDEBOARD_CONFIG_OVERRIDES is not specified in the environment.

The following config files will be loaded, if they exist, in order:

Ex: Using a single absolute path

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="/home/robruana/sideboard/development.ini"

The following config files will be loaded, if they exist, in order:

Ex: Using a single relative path

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="development.ini"

The following config files will be loaded, if they exist, in order:

Ex: Using a relative path with -defaults suffix

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="test-defaults.ini"

The following config files will be loaded, if they exist, in order:

Rationale

Why load /etc/sideboard/*.cfg first?

This is a common pattern in almost all command line and server software. Consider good old bash, first it loads the system-wide /etc/profile before loading your personalized ~/.bash_profile (there's a lot more going on here, but that's the simple explanation).

The system-wide settings in /etc should provide sane defaults. The files under /etc are loaded first so they can be overridden by subsequent files.

Why a special case for -defaults suffix?

This facilitates a common development pattern in which development-defaults.ini is checked into a git repository, while development.ini is added to .gitignore. This provides developers with a set of sane defaults, while allowing them to customize their dev environment without cluttering the git repo.

Similarly test-defaults.ini and test.ini are often used to configure a test environment.

Adding a special case to handle the -defaults suffix makes setup easier by favoring convention over configuration.

Why treat relative paths differently?

Again, by favoring convention over configuration we can save a lot of effort. Sideboard and each of it's plugins use the development-defaults.ini pattern. By searching for paths relative to the sideboard directory AND each plugin directory, we can load each of their config files by specifying development-defaults.ini only once.

Why not use a command line switch?

We certainly could! A command line switch to specify config files could work in conjunction with the environment variable to further override default settings. So a command line switch is a possible future addition, but isn't necessary at the moment.

Why do this at all?

The motivation for this proposal is to make it easier to load settings from test-defaults.ini when running unit tests. A test environment usually requires a configuration that is different from a development environment.

During development, you usually want to your server to listen on a known port and auto-reload when you make code changes. You usually want to use the same database that you use in production (like Postgres).

development-defaults.ini

[cherrypy]
engine.autoreload.on = True
server.socket_port = 8282

[secret]
sqlalchemy_url = "postgresql://rams_db:rams_db@localhost:5432/rams_db"

When running unit tests, you DON'T want your server to auto-reload when you make code changes. In order to avoid address conflicts you DON'T want to listen on a known port. It's common to run unit tests against a sqlite database, because tests may repeatedly create and destroy a database. (For functional tests, you probably still want the same database that you use in production)

test-defaults.ini

[cherrypy]
engine.autoreload.on = False
server.socket_port = 0

[secret]
sqlalchemy_url = "sqlite+pysqlite:///file.db"

Without any way to specify a different test configuration, developers must dynamically patch development-defaults.ini in code.

RobRuana commented 7 years ago

NOTE

For anyone that's read The Twelve-Factor App, instead of putting our config in files, we may want to consider passing the entire config to sideboard via environment variables. This is something I've never tried, but it's supposed to make containerized deployments easier.

We've already done a pretty good job of separating config from code, so we're halfway there. And loading config entirely from the environment is largely orthogonal to this task – the two modes of configuration shouldn't interfere with each other.

kitsuta commented 7 years ago

This sounds good! However, doesn't this just mean that you'd have to change a line in Sideboard's config whenever you want to run unit tests? Do you think it's sensible to also implement conventions for test configs specifically?

Another thing we could do to handle that (which would out-of-scope for this issue) is to implement a way to 'refresh' config, so that you can dynamically switch your config files and reload whatever from them can be reloaded. This has the bonus of moving us towards dynamically-changing config -- a lot of our config in the RAMS plugin is stuff we would like to be able to change on-the-fly, and the current config setup of statically-loaded .ini files hinders that.

RobRuana commented 7 years ago

You wouldn't have to change any lines in sideboard's config to run unit tests, though you would have to do a little extra work before invoking them. There are several options for configuring the test environment. One way would be to specify the environment variable right at the command prompt:

$ SIDEBOARD_CONFIG_OVERRIDES="test-defaults.ini" python -m pytest

Alternately, we could have a run_tests.sh script that sets up the environment before executing the tests:

#!/bin/bash

export SIDEBOARD_CONFIG_OVERRIDES="test-defaults.ini"
python -m pytest

Finally, we could specify our environment variables in tox.ini:

[tox]
envlist = py27
[testenv]
setenv =
    SIDEBOARD_CONFIG_OVERRIDES=test-defaults.ini
deps= -rrequirements.txt
commands=
    coverage run -m py.test
    coverage report

Another reason for doing all this?

As a plugin developer, I'd like to be able to specify my own configuration when I run unit tests. As it stands right now, I must import patch_session from sideboard to create a test DB. Furthermore, I must hot-patch the config object in code to change config settings.

I would much prefer passing the configuration via a command line switch or an environment variable. This issue proposes using an environment variable specifically because I believe Eli has done some work on it already.

Regarding on-the-fly config changes

I don't think this would specifically help in that regard. I think we are always going to have to restart sideboard anytime we change the config :(

RobRuana commented 7 years ago

There's also the possibility that I am not understanding something about the system!

There may already be a way accomplish this task, and I've just been Doing Things the Hard WayTM

:sweat_smile:

EliAndrewC commented 7 years ago

This proposal looks great! It's a little bit different than what we've already implemented at my day job, which I've just pushed out to #67.

What we did in our implementation of SIDEBOARD_CONFIG_ROOT was to allow an environment variable to define where to look for config files. In other words, Sideboard usually looks for config files in this order:

And plugins usually look in this order:

The PR at #67 changes this so that the above behavior is what happens by default, but the SIDEBOARD_CONFIG_ROOT environment variable will be used instead of /etc/sideboard for the final two files. This allows us to not only supply test config files, but also to theoretically run multiple Sideboard server processes on the same server, because we could have e.g. /etc/sideboard1 and /etc/sideboard2 and whatnot.

Rob is correct that the approach in #67 differs from the standard convention used by bash and other Unix programs generally. In this case we're loading the development files before the "global" files. We did it this way because it gave us an easy way to kill two birds with one stone; we can support unit test config files and running multiple Sideboard with the same SIDEBOARD_CONFIG_ROOT feature.

I don't object to doing it differently; I think Rob's proposal is more idiomatic, so if we feel it's worth putting in the work then I think it would be more intuitive to newcomers.

RobRuana commented 7 years ago

Okay, I think your implementation of SIDEBOARD_CONFIG_ROOT ultimately accomplishes a slightly different - but totally worthwhile - goal.

So, I'm thinking we should keep the current implementation of SIDEBOARD_CONFIG_ROOT and add a SIDEBOARD_CONFIG_OVERRIDES environment variable that functions as I've described above.

Thoughts?

EliAndrewC commented 7 years ago

I like it! Adding a SIDEBOARD_CONFIG_OVERRIDES offers a much simpler approach if all you care about is adding a test file to override the default settings but then we still get to keep the SIDEBOARD_CONFIG_ROOT option if people want the heavier option.

RobRuana commented 7 years ago

While working through this issue I realized the load order is going to be slightly different than outlined above. All sideboard configs will be loaded first, followed by all configs for each plugin, in order.

Ex: Using a single relative path

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="development.ini"

The following config files will be loaded, if they exist, in order: sideboard configs

uber configs

reports configs