Update 'CKAN Configuration Options' docs

[vitorbaptista]

I haven't used autodocs for this either because I couldn't think about a good place to put the file with all the documentations to be parsed. As, AFAIK, we can't use the docs inside the .ini files, it felt duplicated to try and create another file just to gather all config options and document them. That feels like the same as simply creating them directly into the .rst file.

Fixes #534

[tobes]

quick thoughts

ckan.site_about - this is broken and genshi based remove any ${...}

ckan.legacy_templates
extra_template_paths
extra_public_paths
ckan.template_head_end
template_footer_end

• add note these options will be removed soon for legacy use only

site_title in wrong section move to with description

Authorization Settings - remove the whole section

[vitorbaptista]

Thanks for the comments, @tobes.

Could you explain me why we should remove Authorization Settings? I still see it used in https://github.com/okfn/ckan/blob/master/ckan/model/authz.py#L341. Is it going to be removed for 2.0? Maybe we should remove this part of the documentation whenever we remove that code? Also, remove it from the development.ini...

[vitorbaptista]

ckan.site_about - this is broken and genshi based remove any ${...}

Is this a note I should add to the documentation, or you're telling me to remove ${...} from the examples?

[tobes]

Authorization Settings?

this is old crap that needs removing at some point. The auth stuff changed totally in 2.0. some old code remains and will eventually be removed

or you're telling me to remove ${...} from the examples?

this

[vitorbaptista]

@tobes it should be good to merge now, could you double check?

[tobes]

a)

ckan.legacy_templates
extra_template_paths
extra_public_paths
ckan.template_head_end
template_footer_end

add note these options will be removed soon for legacy use only

you have added a note saying they deprecated - this is wrong it should say they are for legacy use only and should not be used

b)
The options need to be reordered and the sections changed so that useful options are at the start etc

c) ckan.site_about there is no default value if it is empty we use a template home/snippets/about_text.html also markdown is only for jinja2 templates not legacy Genshi ones

d) openid_enabled openid may be broken so I'm not sure if this should be here? someone else may have a view

@seanh will ultimately be doing the merge

[vitorbaptista]

@tobes I've just pushed your last suggestions

[seanh]

That's a shame that autodoc can't parse the ini file. If that's the case, then I think we should document each config option in Sphinx and not document any of them a second time with comments in the deployment.ini_tmpl, we should delete those.

But maybe we should make sure that every option appears in deployment.ini_tmp with a default value, even if commented out? We could add a unit test to enforce this, which would make sure new config options get added.

As for the Sphinx docs, I really think it'd be nice to do this with autodoc. Is there really not One Place in the source code where all the config options are defined? There probably should be..

[seanh]

I haven't looked through them all but it looks like maybe every ckan.* config option should be listed in app_globals.py. Most of them are in there already. Could they all have docstrings in there, and be pulled out by autodoc?

Things that are not in app_globals.py are:

Options from libraries that we use, e.g. sqlalchemy.url. There's a limited number of these that doesn't seem likely go grow very fast. Maybe they can be added to app_globals.py? Or maybe these ones can just go into configuration.rst directly. As discussed, we don't need to document every config option from a library that we use, just the more important ones.

Some CKAN options are missing from app_globals.py, including: ckan.dataset.show_apps_ideas, ckan.preview.direct, ckan.preview.loadable, ckan.feeds.author_name, and more (I didn't check them all). Could it be all the ckan.*.* ones that are missing? Anyway, I'm guessing it would be good to add these into app_globals.py.

I need to educate myself about what app_globals is for. I think it's a Pylons thing. I'll have a look.

Some options in configuration.rst are not actually used in CKAN at all, even though they may exist in deployment.ini_tmpl. For example, rdf_packages. These should be deleted from both configuration.rst and deployment.ini_tmpl.

Some other config options are only used in the legacy templates, these should probably just be deleted from both configuration.rst and deployment.ini_tmpl as well, we agreed a while ago to delete the legacy templates before releasing 2.0 we just haven't got round to it yet.

Some options seem to be named wrong in configuration.rst, e.g. shouldn't recaptcha be ckan.recaptcha? With autodoc this wouldn't happen.

[seanh]

@tobes Do you know what this mappings dict in app_globals.py is for? It doesn't appear to be used. https://github.com/okfn/ckan/blob/master/ckan/lib/app_globals.py#L19

[seanh]

Alright. This is a bit of a mess. IMHO ideally we wouldn't have each config value defined in three or four different places (deployment.ini_tmpl, config_details dict in app_globals.py, and doc/configuration.rst file), we need to try and cut this down.

Maybe we can define the config settings in app_globals.py with docstrings, and pull the docstrings into configuration.rst using autodoc? The problem with this is the config settings in app_globals.py are defined in a dict, config_details, and you can't add docstrings to dictionary items.

You can add docstrings to attributes in Python, as long as those attributes are simple assignments at the top-level of a module, class or __init__() method. So maybe app_globals.py can be changed to use attributes instead of config_details? We could assign the attributes at the top level of the _Globals class, like this:

class _Globals(object):

    search_facets = {'default': 'groups tags res_format license',
            'type': 'split', 'name': 'facets'},
    '''Define the facets that appear on the dataset search page.'''

    ...

    def __init__(self):
        ...

    ...

With this approach, I think we could add a unit test or other code to enforce that every config setting defined in _Globals has a docstring, and we could potentially add code to look for unused configs as well. Then the only other place where config settings are repeated is in deployment.ini_tmpl and we could have a policy of not double-documenting them in there.

You can't have dots in python attribute names, so maybe we can say that a config setting called ckan.site_title becomes an attribute called just site_title ie. strip the ckan.. Bit of a shame that this will show up in the sphinx docs as site_title not ckan.site_title, but maybe we can hack that somehow, or else just put a note at the top of the docs page explaining it.

Unfortunately some config settings have even more dots in their names, e.g. ckan.preview.direct etc. We could just change all these to eg. ckan.preview_direct and from now on say no dots in config setting names.

Another problem is how to we enforce that every config setting has to have an attribute in _Globals? There are parts of code in CKAN that just do pylons.config.get('ckan.some_random_setting') and some_random_setting is not mentioned anywhere in app_globals.py or anywhere else in CKAN. If we want to have some sort of automated check to enforce that all config settings have docstrings then I'm not sure how we'll catch these ones.

There's also quite a lot going on with some config settings in environment.py. For example if ckan.site_id is not defined in the config file, it looks for an operating system environment variable called CKAN_SITE_ID. We may have to clean this up to make sure that environment.py is just reading config settings, and not defining new ones or new ways of setting them.

@vitorbaptista @tobes This may all be a stupid idea. Let me know what you think, maybe there's a better solution, or maybe there's nothing to be done and we simply have to document config settings directly in configuration.rst and keep it up-to-date manually.

For details about attribute docstrings in Python and autodoc see:

http://www.python.org/dev/peps/pep-0258/#attribute-docstrings

http://sphinx-doc.org/ext/autodoc.html

[seanh]

Another problem is how to we enforce that every config setting has to have an attribute in _Globals? There are
parts of code in CKAN that just do pylons.config.get('ckan.some_random_setting') and some_random_setting is
not mentioned anywhere in app_globals.py or anywhere else in CKAN. If we want to have some sort of
automated check to enforce that all config settings have docstrings then I'm not sure how we'll catch these ones.

I guess we could try to monkey-patch the __getattr__ of the pylons.configuration.PylonsConfig object if we wanted to do this. Make it crash or throw a warning if someone tries to access a config setting that doesn't have an attribute and docstring in _Globals.

[tobes]

sometimes I think github isn't the place for these discussions as some people may not see them.

I'd like more autodocumentation eg plugins.toolkit maybe we could do this sort of stuff via paster? the toolkit has import circular nightmare issue maybe these will slowly resolve as refactoring happens.

as far as config options maybe app_globals is a good place although some eg openid feel they don't belong there.

we can definitely sortthe config being used everywhere stuff, perhaps by removing the config or hiding it cunningly - maybe just have a coding standards test that chack for it being imported etc.

[seanh]

@tobes One thing that is still confusing me is why we add all these config settings (the ones in the ckan.lib.app_globals.py:config_details dict) to pylons.app_globals instead of to pylons.config. Especially given that app_globals is not thread-safe and meant to be used read-only, which I guess is why we've implemented our own mutex and lock stuff in app_globals.py.

If I understand it all right (and I haven't tested to verify this) it looks like we've implemented our own system where the value for a config option may come from the default hardcoded in the config_details dict, or from the config file (which overrides the config_details dict), or from the system_settings db table (which overrides both the hard-coded default and the config file), and the Reset button resets a value to the hard-coded or config file default (ie. clears the option from the system_settings table). This all seems good but I just don't understand why it's done with app_globals rather than pylons.config (in which case all our custom logic for this would belong somewhere in ckan.config probably).

[tobes]

@seanh

The reasons for the config vs globals are as follows

config is just whatever gets added via the .ini and is unprocessed (this could be changed) because of this config stuff tends to end up in code as if asbool(config.get('opt', False)): whereas the globals stuff tends to just be if g.opt: which is more readable.

config is not available to the templates whilst g is available.

having stuff in 2 places is always confusing so that is why I suggest we just use g

[seanh]

having stuff in 2 places is always confusing so that is why I suggest we just use g

Hmm. Didn't we have the stuff in one place (the pylons.config as provided by pylons) until we started adding them to app_globals as well? Now we have them in two places. I don't think it's possible to just use g and have them in one place, because pylons will always make them available in config as well, and devs are likely to use config since it's the normal pylons way to access config settings. We'll have to add a new rule to the ckan coding standards, "Don't use config like a pylons app normally would, use g, because ckan is different", the less rules like this we have the better, I think. CKAN is "different" in all sorts of surprising ways and I don't think the overall effect is a benefit.

If asbool(config.get('opt', False)) is too much boilerplate and not accessible from templates, can't we add a simple helper function to do it for us, and make it available as a template helper?

So I'm thinking maybe we should look into removing this stuff from app_globals and doing what we want based on config.

P.S. In reading about this I found out that calling it g is deprecated in Pylons, it's supposed to be app_globals now: http://pylonsbook.com/en/1.1/exploring-pylons.html#app-globals-object (which I prefer because it's much clearer, I see some CKAN code accessing something called app_globals, I look for it and find app_globals.py, versus trying to find wtf g is)

[tobes]

I agree that g is not very clear though I'm used to it now same way as c is tmpl_context or similar name. g is likely to be used in templates etc due to it's nice shorthand.

A problem with using config is it is text and has no defaults. if we do asbool(config.get('opt', False)) in the code other people may use asbool(config.get('opt', True)) and that is going to be hard to track down so I'd like to have the config sanitized in some way g was a nice way to get this done.

Also I'm keen to get ckan to a point when we can switch frameworks. Pylons is unsupported and must have huge security issues by now I'd imagine. I think the app_globals code helps with this.

g is also something provided by pylons not something we have made-up ourselves - though it maybe quite ckanized now.

It was also agreed long ago about config being kept out of the templates due to ease of abuse.

[seanh]

@tobes I think this is another one of those interesting discussions that should move to the dev list. I'll send an email.

@vitorbaptista While we argue about this do you want to make the changes in configuration.rst that I suggested? Some options mentioned in configuration.rst are not actually used in CKAN, or only used in legacy templates, and should be removed. Also remove them from deployment.ini_tmpl. Some options nameed wrong in configuration.rst. Also may be worth another check that you got all the options that CKAN uses in configuration.rst -- code may access options via pylons.app_globals, pylons.g or pylons.config (and sometimes these may be imported with from pylons import *) -- gotta catch 'em all! :) This will not be a waste of time, as any docs you write will get cut-pasted into docstrings at some point, also it's quite likely that any code refactoring Toby and I can agree on will be delayed until 2.1 anyway so we will need the "manual" docs for 2.0.

[amercader]

How does this relate to #746? It seems to me that they are quite related as deployment.ini_tmpl should match whatever documentation is in configuration.rst, so I'll wait until this is done (or feel free to do both at the same time)

[amercader]

@vitorbaptista Do you need more work on this or can it be reviewed?

[vitorbaptista]

@amercader I'll finishing writing the configuration variables discussed in #534 today, and then this might be reviewed.

[amercader]

Closing this one as I needed to make changes, current is now #833