Allow setting defaults through a config file.

[sunu]

Partly addresses #146

[codecov-io]

Current coverage is 85.32%

Merging #279 into master will decrease coverage by -0.17% as of 7be06ac

@@            master    #279   diff @@
======================================
  Files           40      41     +1
  Stmts         4384    4429    +45
  Branches       567     576     +9
  Methods          0       0       
======================================
+ Hit           3748    3779    +31
- Partial        143     148     +5
- Missed         493     502     +9

Review entire Coverage Diff as of 7be06ac

Powered by Codecov. Updated on successful CI builds.

[kmike]

Hey @sunu,

Thanks for the PR! There are some design decisions to make:

1. Should we use yaml? Currently other Splash config files are .ini (e.g. proxy profiles). ini ConfigParser is in standard library, so it doesn't require a dependency. It also supports merging several config files out of the box, so you can have defaults.ini merged with /etc/splash.cfg merged with ~/.config/splash.cfg.

   Are there yaml features we use? YAML is a more complex beast - e.g. in your current implementation user can execute arbitrary Python code by creating a YAML config (see http://nedbatchelder.com/blog/201302/war_is_peace.html).

2. I think we should support config files in some standard locations: /etc/splash.cfg; C:\\splash\splash.cfg; ~/.config/splash.cfg ($XDG_CONFIG_HOME); ~/.splash.cfg; maybe splash.cfg in a current folder (assuming we go with .ini; for yaml all files should be .yaml).

3. It should be possible to pass a path to config file to Splash at startup.

[sunu]

Currently other Splash config files are .ini (e.g. proxy profiles). ini ConfigParser is in standard library, so it doesn't require a dependency. It also supports merging several config files out of the box, so you can have defaults.ini merged with /etc/splash.cfg merged with ~/.config/splash.cfg.

Yes, this makes a lot of sense. I just went for yaml because I find it somewhat more readable. Using ini files makes sense if we're already using them.

YAML is a more complex beast - e.g. in your current implementation user can execute arbitrary Python code by creating a YAML config (see http://nedbatchelder.com/blog/201302/war_is_peace.html).

Good to know. Thanks. Will remember to use safe_load with yaml from now on.

I think we should support config files in some standard locations: /etc/splash.cfg; C:\splash\splash.cfg; ~/.config/splash.cfg ($XDG_CONFIG_HOME); ~/.splash.cfg; maybe splash.cfg in a current folder (assuming we go with .ini; for yaml all files should be .yaml).

How is this implemented? Should we hard-code the possible standard locations?

It should be possible to pass a path to config file to Splash at startup.

OK. Will add an option for that.

[sunu]

What I wanted to ask earlier is that in this implementation we essentially read the config file every time settings is imported. So doesn't that count as blocking code? Or does it not matter because of the relatively small size?
And suppose if it was a more time-taking read operation. What would be the work around to avoid blocking here?

[kmike]

Hey @sunu!

How is this implemented? Should we hard-code the possible standard locations?

Yes.

What I wanted to ask earlier is that in this implementation we essentially read the config file every time settings is imported. So doesn't that count as blocking code? Or does it not matter because of the relatively small size? And suppose if it was a more time-taking read operation. What would be the work around to avoid blocking here?

For config files it makes more sense to read them once and then return results from memory. Blocking doesn't matter because it is very quick to read a config file, and because it is usually read at startup when Splash is not serving requests yet.

True non-blocking reads from disk are rarely used because usually you can't make reads from disk more efficient by sending several read requests in parallel.

[sunu]

Hello @kmike
I've updated the PR. Does this approach look ok to you? I'm using __builtin__ module to make the config_path option available in splash.config. It's kind of an hacky solution to the problem, but I couldn't find any other way to do it.

Also, should the config keys be case insensitive? I've overridden the default behavior to make them case sensitive, but let me know what the expected behavior is.

[kmike]

Hey @sunu,

I think using __builtins__ is a non-starter; another solution is needed even if it would require writing more code or not supporting settings.OPTION_NAME syntax.

[sunu]

Hey @kmike,
I have replaced the use of builtin module by using a global variable now.

[kmike]

this looks brittle.. It will require changing more code, but maybe we can make this path a Settings constructor parameter and don't create a global settings variable at module level? We can create it in server startup code (when the path is known) and pass it down to all components via function/method/constructor arguments instead.