added SIDEBOARD_CONFIG_ROOT environment variable#67
Conversation
| If the SIDEBOARD_CONFIG_ROOT environment variable is set, its contents will be returned instead. | ||
| """ | ||
| config_root = os.environ.get('SIDEBOARD_CONFIG_ROOT', '/etc/sideboard') | ||
| assert os.path.isdir(config_root) and os.access(config_root, os.R_OK), '{!r} directory is not readable'.format(config_root) |
There was a problem hiding this comment.
This seems to be failing if /etc/sideboard does not exist. Can we maybe do an existence check before the assert? Or, instead of failing, maybe issue a warning?
There was a problem hiding this comment.
Ooh, good catch. This happens to have not bitten us at my day job because of how we provision our Vagrant environments, but it would totally bite anyone who happened to provision their Vagrant boxes without actually installing Sideboard!
The problem with logging a warning is that we haven't actually configured logging yet. So how about this:
- if the
SIDEBOARD_CONFIG_ROOTenvironment variable is NOT set, then we don't do anything if/etc/sideboarddoesn't exist because that's a totally valid and expected use case - if the directory DOES exist but we can't read it, then we treat that as an error and raise an exception
- if the
SIDEBOARD_CONFIG_ROOTenvironment variable IS set, then we raise an exception if it doesn't exist or we can't read it
That behavior seems intuitive to me; if you haven't set it then you might not care, but if you do set it then we'd expect it to be valid.
How does that sound?
There was a problem hiding this comment.
Perfect! I feel that is exactly how you'd expect it to work!
There was a problem hiding this comment.
Ok, sounds great - I'll push out a fix with some associated unit tests in the next few minutes.
| raise AssertionError('cannot find {!r} directory'.format(config_root)) | ||
| elif os.path.isdir(config_root) and not os.access(config_root, os.R_OK): | ||
| raise AssertionError('{!r} directory is not readable'.format(config_root)) | ||
| return config_root |
There was a problem hiding this comment.
Done! Take a look and let me know what you think.
|
Looks great, you even managed to increase out code coverage by 0.08%. Amazing! Travis is failing on the py3.* builds, but only because of the changes in newer versions of cherrypy. |
added SIDEBOARD_CONFIG_ROOT environment variable
3 participants
We implemented this locally to make it easier to run unit tests on different environments. I accidentally neglected to push this up with my recent "resync back to the public repo" work.
This is relevant to #66 which I'll comment on in a second to explain how it relates to what's been proposed there.