Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2842] New CKAN config object, decoupled from Pylons #3163

Merged
merged 8 commits into from Jul 14, 2016

Conversation

Projects
None yet
3 participants
@amercader
Copy link
Member

commented Jul 13, 2016

This PR includes a bunch of changes that provide a common configuration object for CKAN that works both on Flask and Pylons.

Summary:

  • Changes in the way the environment is set up to make it more generic and pass the config options from the ini file to Flask (048632d)
  • A new dict-like, thread-local safe (using Werkzeug's Local object) CKAN config object, that apart from holding the config values proxies any changes to the Flask and Pylons own config objects (ad46dce)
  • Update all core imports of pylons.config to ckan.common.config (These are the bulk of the changes in a77ff9c)
  • Add config to the plugins toolkit (37ecbe8)

Discussion:

Both Flask and Pylons config objects are dict-like objects with key/value configuration options. But the Flask one is bound to the application and so it's only available when there is an application context available. This means that it can not support the way in which we were using the Pylons config, which was essentially available everywhere, on the CLI, tests, etc. In order to not change completely the way the application is structured and how the configuration is accessed in all parts of the code it seemed a good approach to have our own dict-like config object that was available in the same way the Pylons config object is.

At the same time we don't want to have different sets of configurations for stuff relevant to CKAN, Flask or Pylons (eg ckan.site_url, PROPAGATE_EXCEPTIONS or pylons.app_globals respectively). The CKAN config objects proxies any changes to the Flask and Pylons config objects (if they are available) so you can still configure all three applications using the same ini file (or env vars).

Moving forward this should be the only configuration object used in all code unless we really
need to access the underlying Flask or Pylons objects for some reason.

Extensions should be encouraged to use toolkit.config (or ckantoolkit.config, PR incoming).

amercader added some commits Jun 23, 2016

[#2842] Refactor evironment loading, initialize Flask configuration
Call `load_environment` before making both stacks rather than from the
Pylons one. Call the app_globals code on the common environment code,
not just on the Pylons stack.

Update the Flask config object with all the CKAN values.

Don't pass explicitly the configuration object to the `load_all` plugins
function, use the one in common.
[#2842] Wrap Pylons requests in a Flask application context
On this particular branch this is needed so the common CKAN config
object can forward config options to the Flask app config object, but
this will be required anyway as more Flask features need to be available
during a Pylons request (see 62f55d2).
[#2842] New CKAN config object
Rather than rely on the Pylons (or Flask) config object we define our
own in ckan.common. This is a dict-like object (so fully backwards compatible)
that also proxies any changes to the Flask and Pylons configuration objects
(if they are available)

This should be the only configuration object used in all code unless we really
need to access the underlying Flask or Pylons objects for some reason.

The `ckan.common.config` instance is initialized in the `load_environment`
method with the values of the ini file or env vars.

This is actually a proxy to a property from a Werkzeug Local object, meaning
that `config` is thread-local safe, like its Flask and Pylons counterparts.

See http://werkzeug.pocoo.org/docs/0.11/local/

I tried to separate all Pylons specific stuff (things like `pylons.paths`,
`routes.map` etc) to Pylons own config object to keep the main config object
clean but it proved too difficult, not only because we access these keys from
different parts of the code (which can be solved) but also because when
clearing the config (done on the tests) we lose keys that were added on
`load_environment` and we would need to keep track of those.
Merge branch 'master' into 2842-common-config
Conflicts:
	ckan/config/middleware/flask_app.py
	ckan/config/middleware/pylons_app.py
@@ -356,6 +366,7 @@ def _add_ckan_admin_tabs(cls, config, route_name, tab_label,
# update the config with the updated admin_tabs dict
config.update({config_var: admin_tabs_dict})

# import ipdb; ipdb.set_trace()

This comment has been minimized.

Copy link
@davidread

davidread Jul 14, 2016

Contributor

oops

@davidread

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

Looks excellent to me.

@wardi

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

Looks good to me too.

@wardi wardi merged commit f0ab2b3 into master Jul 14, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@amercader amercader deleted the 2842-common-config branch Sep 22, 2016

@amercader

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2016

See also ckan/ckantoolkit#5 for extensions that want to support multiple CKAN versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.