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. Thanks for the review. I think you're right. It makes more sense to use composite than Inheritance here. Let's do the objective way because in future we could add more common configurations for kimchi and plugins. After we add that kind of configurations, we could change to Inheritance because
On 03/04/2014 03:51 PM, Zhou Zheng Sheng wrote: the attributes are in parent object.
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