959728 - Refactor config system

Status: RESOLVED FIXED

(Webtools Graveyard :: DXR, defect)

Description

[Erik Rose [:erik][:erikrose]]

* There's way too much repetition between Config and TreeConfig.
* Both classes are too dependent on an FS artifact existing. That makes us lay (more) files down in temp dirs in our test harnesses.
* They call sys.exit(), which is evil.
* They're super verbose.
* They don't convert things to the right type, so we end up (for example) having to cast nb_jobs to an int elsewhere.

Comment 1

[Erik Rose [:erik][:erikrose]]

And let's not have a config.ini at one point and a config.py at another. Let's unify them.

Comment 2

[Nick Cameron [:nrc]]

config.ini, config.py AND dxr.config

Comment 3

[Erik Rose [:erik][:erikrose]]

There's no such thing as config.ini; that was my sleepless brain malfunctioning. :-)

Comment 4

[Erik Rose [:erik][:erikrose]]

For (loose) example, it could look something like this:

config = Config(...,
                {'DXR': [('nb_jobs', 4),
                         ('plugin_folder': '%(dxrroot)s/plugins')]})

Comment 5

[Erik Rose [:erik][:erikrose]]

With unified config files, we can make make_app() work during the build process. Then we can use url_for() everywhere instead of cobbling together our own paths. This will probably bear on request-time rendering as well.

Comment 6

[Erik Rose [:erik][:erikrose]]

configobj (http://www.voidspace.org.uk/python/articles/configobj.shtml) looks pretty good, with validation, var-args-style sections, type conversion, and painless callsites. You define your "schema" in terms of an ini file (or, presumably, string or at least StringIO), so we keep a (even more) readable self-documenting code. You can even pass in a dict to construct one, which will let the test harness stop constructing so many files: ConfigObj(indict=some_dict).

We'd have to make a small change to our existing config files, placing the trees under a "[trees]" section, but that seems worth it to me.

Comment 7

[Erik Rose [:erik][:erikrose]]

configman won't work off the shelf because it doesn't support arbitrary numbers of tree sections unless they're first themselves named in another directive: trees = tree1, tree2, .... It's also pretty solidly overkill for DXR.

Comment 8

[Erik Rose [:erik][:erikrose]]

With configobj, how are we going to get/check plugin-introduced options? Worst case, we can have plugins expose additional configuration to be inserted into the configspec and do that iff they're enabled. But I'm not sure how to deal with the case where they're enabled for only certain trees.

Comment 9

[Erik Rose [:erik][:erikrose]]

When the config system gets refactored, this means we can possibly remove our dependency on mock, which we (will have) needed to make a TreeConfig without laying down a file on the FS.

Comment 10

[[github robot]]

Commit pushed to es at https://github.com/mozilla/dxr

https://github.com/mozilla/dxr/commit/8ab05fa6f7f0f7f66e80a620f5a718ed443ff239
Make Config no longer dependent on filesystem objects. Ref bug 959728.

This saves us a file write during SingleFileTestCases.

build_instance will likely change to take a dict-like object later, making it even more manipulable, once we've converted to configobj.

Comment 11

[Erik Rose [:erik][:erikrose]]

Here's a sketch of what a configobj-savvy dxr.config might look like:

    # Global stuff here, no longer in a [DXR] section:
    target_folder = /somewhere/target
    es_index = dxr_test_{format}_{tree}_{unique}

    [mozilla-central]  # a tree
    build_command = /bin/true
    enabled_plugins = buglink clang python pygmentize

        [[buglink]]  # configuration for a plugin
        name = Bugzilla
        url = https://bugzilla.mozilla.org/show_bug.cgi?id=%s

Plugin configs would go into their own sections, unprefixed by the wordy "plugin_buglink_", etc. Perhaps each plugin (whether enabled or not) can insert a [__many__][the_plugin_name] section into the configspec so we can validate settings and provide defaults.

Comment 12

[Erik Rose [:erik][:erikrose]]

Also have a look at https://github.com/henriquebastos/python-decouple, which might give us env var equivalents to config file settings for free.

Comment 13

[Erik Rose [:erik][:erikrose]]

Keep in mind that, for PaaS-type deployments (like AWS), we'll want to have at least a pointer to our config file—and maybe all the config itself—in env vars.

Comment 14

[Nick Cameron [:nrc]]

Another use case (and to check it works post-refactor): getting the list of enabled plugins at runtime. This is immediately useful for constructing the filter menu, but I suspect it will also be required for other things in the future around more dynamic construction of pages, etc.

Comment 15

[Erik Rose [:erik][:erikrose]]

configman knows how to take an ini file, complete with nesting, and express them as env vars (and vice versa).

Comment 16

[Erik Rose [:erik][:erikrose]]

When you do this, have a look at source_folder and object_folder, see what they really are for, and clarify or at least better document the design.

Comment 17

[Erik Rose [:erik][:erikrose]]

Some other problem we'll solve:
* Config values get validated late (after a 2-hour build phase completes, say)
* Unrecognized values don't throw an error. (These signal misspellings or misplacements, like the time we put "es_hosts" in a tree rather than up top and then wondered why it was trying to connect to localhost.)

Comment 18

[[github robot]]

Commit pushed to es at https://github.com/mozilla/dxr

https://github.com/mozilla/dxr/commit/b5fc2f3b88e23369ba471b5214824c97b2cc088f
Redo config subsystem. Fixes bug 959728.

For deployers:

* Improved validation of known options. Unknown plugins and bad regexes are caught early, before spending 2 hours building a tree.
* Started rejecting unknown options, so we catch obsolete things or spelling errors that would otherwise not have their intended effect.
* Merge disable_workers and nb_jobs to create the `workers` setting. Set it to 0 to do what disable_workers used to. It now defaults to the number of CPUs and no longer crashes if left unspecified.
* Renamed wwwroot to www_root for consistency. Changed default from "/" to "". I don't know how it was not spitting out double slashes before.
* Remove the -j option from dxr-build.py. There's nothing special about that option that merits a commandline flag. We should have it for all or none. (We will probably support at least env vars for all options down the road.)
* Comments are parsed differently now: any # in a config value is interpreted as the beginning of a comment, unless the value is surrounded by quotes.

For plugin authors:

* Start passing the name of the plugin into the TreeToIndex so we no longer have to repeat the entrypoint name in 2 places.
* Added plugin_config() method to TreeToIndex, FileToSkim, and FileToIndex so you can easily grab your plugin-specific config.
* Plugins now take an optional config_spec kwarg which provides a config schema.
* See how the buglink plugin collapsed thanks to the new declarative validation.

Other improvements:

* Hide useless intermediate values like the root-level enabled_plugins setting.
* config.trees is now an OrderedDict rather than a list, so we don't have to flip through it every time to find the tree we want.
* <some tree>.enabled_plugins is a list rather than an OrderedDict. There's no need for the keys now that plugins know their own names, so it reduces the noise of us calling .values() all the time.
* ignore_patterns is split into ignore_paths and ignore_filenames when the config file is read, and the combined ignore_patterns is no longer available.
* Stop raising ConfigError in omniglot, as having a non-GitHub git remote has nothing to do with the configuration file.
* Stop calling sys.exit() from the Config object. Raise a ConfigError like a good boy. ConfigErrors format themselves nicely and so double as non-horrible user-facing errors. We might polish up error reporting later, suppress the traceback, and reporting multiple errors at once.
* You can pass in config as a dict. We'll see if that turns out to be useful, but we get it for free.

This will undergo another round of changes when request-time rendering comes, but this cleans it up a lot.

This does not unify the index-time and request-time configs, but we'll do what we can about that shortly. There are essential differences, like that dynamic defaults (generated_date) are frozen in time at request time.

Comment 19

[[github robot]]

Commits pushed to master at https://github.com/mozilla/dxr

https://github.com/mozilla/dxr/commit/8ab05fa6f7f0f7f66e80a620f5a718ed443ff239
Make Config no longer dependent on filesystem objects. Ref bug 959728.

https://github.com/mozilla/dxr/commit/b5fc2f3b88e23369ba471b5214824c97b2cc088f
Redo config subsystem. Fixes bug 959728.