Re: [Kimchi-devel] [PATCH 1/2] Add static directory configurations for plugin's ui

on 2014/02/28 17:59, Mark Wu wrote: > Plugin needs static directory configuration for ui requirements, > such as css, js, etc, as what we do for kimchi base. So this > patch refactors the server configuration to make plugin can share > the same code with kimchi. To avoid the warning messages reported from > cherrypy's path checker, it also adds empty ui directory for the > sample plugin, which also can demonstrate plugin's ui directory > sturcture. We can add actual ui files under those direcotries later. > --- > plugins/sample/ui/css/.gitignore | 0 > plugins/sample/ui/images/.gitignore | 0 > plugins/sample/ui/js/.gitignore | 0 > plugins/sample/ui/libs/.gitignore | 0 > src/kimchi/config.py.in | 79 +++++++++++++++++++++++++++++++++++++ > src/kimchi/server.py | 77 ++---------------------------------- > 6 files changed, 82 insertions(+), 74 deletions(-) > create mode 100644 plugins/sample/ui/css/.gitignore > create mode 100644 plugins/sample/ui/images/.gitignore > create mode 100644 plugins/sample/ui/js/.gitignore > create mode 100644 plugins/sample/ui/libs/.gitignore > > diff --git a/plugins/sample/ui/css/.gitignore b/plugins/sample/ui/css/.gitignore > new file mode 100644 > index 0000000..e69de29 > diff --git a/plugins/sample/ui/images/.gitignore b/plugins/sample/ui/images/.gitignore > new file mode 100644 > index 0000000..e69de29 > diff --git a/plugins/sample/ui/js/.gitignore b/plugins/sample/ui/js/.gitignore > new file mode 100644 > index 0000000..e69de29 > diff --git a/plugins/sample/ui/libs/.gitignore b/plugins/sample/ui/libs/.gitignore > new file mode 100644 > index 0000000..e69de29 > diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in > index 2747164..f599c4d 100644 > --- a/src/kimchi/config.py.in > +++ b/src/kimchi/config.py.in > @@ -127,6 +127,85 @@ class PluginPaths(Paths): > self.conf_file = os.path.join(self.conf_dir, '%s.conf' % name) > > > +class AppConfig(dict): > + > + # expires is one year. > + CACHEEXPIRES = 31536000 > + > + def _get_ui_configs(self, paths): > + ui_configs = {} > + for sub_dir in ('css', 'js', 'libs', 'images'): > + ui_configs['/' + sub_dir] = { > + 'tools.staticdir.on': True, > + 'tools.staticdir.dir': os.path.join(paths.ui_dir, sub_dir), > + 'tools.nocache.on': False} > + if sub_dir != 'images': > + ui_configs['/' + sub_dir].update({ > + 'tools.expires.on': True, > + 'tools.expires.secs': AppConfig.CACHEEXPIRES}) > + return ui_configs > + Hi, The patch is good, but I don't like the way it uses inheritance. The KimchiConfig and PluginConfig don't have to be the subclass of AppConfig, and AppConfig is actual just a container for UI related config, but not general configurations. Inheritance expresses a strong "is-a-kind-of" relationship. It seems an overkill to use object encapsulation and inheritance here. May I propose a better way to model these three configs as follow? Basically, the concept is rather simple: kimchi_config = basic_kimchi_config + ui_config(kimchi_paths) plugin_config = basic_plugin_config + ui_config(plugin_paths) So in my opinion it should be a use case of composition, not inheritance. Pseudo code is like this. def _get_ui_config(paths): ui_configs = {} for sub_dir in ('css', 'js', 'libs', 'images'): ... # almose the same as AppConfig.__init__ in you code. return ui_configs def _get_kimchi_config(): c = {'/': {'tools.trailing_slash.on': False, ...} c.update(_get_ui_config(paths)) return c def _get_plugin_ui_config(plugin_name): c = {'/ui/config/tab-ext.xml': {... c.update(_get_ui_config(PluginPaths(plugin_name))) return c class Server(object): def __init__(self, options): ... self.configObj = _get_kimchi_config() def _load_plugins(self): ... for plugin_name, plugin_config in get_enabled_plugins(): ... plugin_config.update(_get_plugin_ui_config(plugin_name)) If you prefer the objective way. Maybe you can consider the following object model.

class UIConfig(dict): def __init__(self, paths): for sub_dir in ('css', 'js', 'libs', 'images'): ... # almose the same as AppConfig.__init__ in you code. class KimchiConfig(dict): def __init__(self): super(KimchiConfig, self).__init__() kimchi_config = {...} self.update(self.kimchi_config) self.update(UIConfig(paths)) class PluginConfig(dict): def __init__(self): super(PluginConfig, self).__init__() plugin_config = {...} self.update(plugin_config) self.update(UIConfig(PluginPaths(plugin_name))) > + > +class KimchiConfig(AppConfig): > + kimchi_config = { > + '/': {'tools.trailing_slash.on': False, > + 'request.methods_with_bodies': ('POST', 'PUT'), > + 'tools.nocache.on': True, > + 'tools.sessions.on': True, > + 'tools.sessions.name': 'kimchi', > + 'tools.sessions.httponly': True, > + 'tools.sessions.locking': 'explicit', > + 'tools.sessions.storage_type': 'ram', > + 'tools.kimchiauth.on': False}, > + '/data/screenshots': { > + 'tools.staticdir.on': True, > + 'tools.staticdir.dir': get_screenshot_path(), > + 'tools.nocache.on': False > + }, > + '/data/debugreports': { > + 'tools.staticdir.on': True, > + 'tools.staticdir.dir': get_debugreports_path(), > + 'tools.nocache.on': False, > + 'tools.kimchiauth.on': True, > + 'tools.staticdir.content_types': {'xz': 'application/x-xz'} > + }, > + '/config/ui/tabs.xml': { > + 'tools.staticfile.on': True, > + 'tools.staticfile.filename': '%s/config/ui/tabs.xml' % > + paths.prefix, > + 'tools.nocache.on': True > + }, > + '/favicon.ico': { > + 'tools.staticfile.on': True, > + 'tools.staticfile.filename': '%s/images/logo.ico' % paths.ui_dir > + }, > + '/help': { > + 'tools.staticdir.on': True, > + 'tools.staticdir.dir': '%s/ui/pages/help' % paths.prefix, > + 'tools.nocache.on': False > + } > + } > + > + def __init__(self): > + super(KimchiConfig, self).__init__(self) > + self.update(self.kimchi_config) > + self.update(self._get_ui_configs(paths)) > + > + > +class PluginConfig(AppConfig): > + def __init__(self, plugin_name): > + super(PluginConfig, self).__init__(self) > + plugin_config = { > + '/ui/config/tab-ext.xml': { > + 'tools.staticfile.on': True, > + 'tools.staticfile.filename': > + os.path.join(PluginPaths(plugin_name).ui_dir, > + 'config/tab-ext.xml'), > + 'tools.nocache.on': True}} > + self.update(plugin_config) > + self.update(self._get_ui_configs(PluginPaths(plugin_name))) > + > + > def _get_config(): > config = SafeConfigParser() > config.add_section("server") > diff --git a/src/kimchi/server.py b/src/kimchi/server.py > index 5475b92..0d02868 100644 > --- a/src/kimchi/server.py > +++ b/src/kimchi/server.py > @@ -29,7 +29,7 @@ from kimchi import config > from kimchi.model import model > from kimchi import mockmodel > from kimchi import vnc > -from kimchi.config import paths, PluginPaths > +from kimchi.config import paths, KimchiConfig, PluginConfig > from kimchi.control import sub_nodes > from kimchi.root import KimchiRoot > from kimchi.utils import get_enabled_plugins, import_class > @@ -57,73 +57,6 @@ def set_no_cache(): > > > class Server(object): > - # expires is one year. > - CACHEEXPIRES = 31536000 > - configObj = { > - '/': {'tools.trailing_slash.on': False, > - 'request.methods_with_bodies': ('POST', 'PUT'), > - 'tools.nocache.on': True, > - 'tools.sessions.on': True, > - 'tools.sessions.name': 'kimchi', > - 'tools.sessions.httponly': True, > - 'tools.sessions.locking': 'explicit', > - 'tools.sessions.storage_type': 'ram', > - 'tools.kimchiauth.on': False}, > - '/css': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': '%s/ui/css' % paths.prefix, > - 'tools.expires.on': True, > - 'tools.expires.secs': CACHEEXPIRES, > - 'tools.nocache.on': False > - }, > - '/js': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': '%s/ui/js' % paths.prefix, > - 'tools.expires.on': True, > - 'tools.expires.secs': CACHEEXPIRES, > - 'tools.nocache.on': False > - }, > - '/libs': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': '%s/ui/libs' % paths.prefix, > - 'tools.expires.on': True, > - 'tools.expires.secs': CACHEEXPIRES, > - 'tools.nocache.on': False, > - }, > - '/images': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': '%s/ui/images' % paths.prefix, > - 'tools.nocache.on': False > - }, > - '/data/screenshots': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': config.get_screenshot_path(), > - 'tools.nocache.on': False > - }, > - '/data/debugreports': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': config.get_debugreports_path(), > - 'tools.nocache.on': False, > - 'tools.kimchiauth.on': True, > - 'tools.staticdir.content_types': {'xz': 'application/x-xz'} > - }, > - '/config/ui/tabs.xml': { > - 'tools.staticfile.on': True, > - 'tools.staticfile.filename': '%s/config/ui/tabs.xml' % > - paths.prefix, > - 'tools.nocache.on': True > - }, > - '/favicon.ico': { > - 'tools.staticfile.on': True, > - 'tools.staticfile.filename': '%s/images/logo.ico' % paths.ui_dir > - }, > - '/help': { > - 'tools.staticdir.on': True, > - 'tools.staticdir.dir': '%s/ui/pages/help' % paths.prefix, > - 'tools.nocache.on': False > - } > - } > - > def __init__(self, options): > make_dirs = [ > os.path.dirname(os.path.abspath(options.access_log)), > @@ -137,6 +70,7 @@ class Server(object): > if not os.path.isdir(directory): > os.makedirs(directory) > > + self.configObj = KimchiConfig() > cherrypy.tools.nocache = cherrypy.Tool('on_end_resource', set_no_cache) > cherrypy.tools.kimchiauth = cherrypy.Tool('before_handler', > auth.kimchiauth) > @@ -214,12 +148,7 @@ class Server(object): > script_name = plugin_config['kimchi']['uri'] > del plugin_config['kimchi'] > > - plugin_config['/ui/config/tab-ext.xml'] = { > - 'tools.staticfile.on': True, > - 'tools.staticfile.filename': > - os.path.join(PluginPaths(plugin_name).ui_dir, > - 'config/tab-ext.xml'), > - 'tools.nocache.on': True} > + plugin_config.update(PluginConfig(plugin_name)) > except KeyError: > continue >