Dan Kenigsberg has posted comments on this change.
Change subject: rest-api: Add the basic server infrastructure
......................................................................
Patch Set 3: (8 inline comments)
partial review
....................................................
Commit Message
Line 39: * Cheetah template files to render responses in json and xml format
do we really need both?
is there no automatic translation between xml to json?
I fear data duplication.
Line 50: Signed-off-by: Adam Litke <agl(a)us.ibm.com>
what tests/rest_override/__init__.py is for?
vdsm/rest subdir should probably be added to pep8's whitelist. and to pep8's.
and a real pyflakes issue should be fixed:
tests/restTests.py:57: local variable 'IndexError' is assigned to but never used
....................................................
File vdsm/clientIF.py
Line 99: def _getServerIP(self, addr=None):
eh, that's back here. I have a deja vu ;-)
....................................................
File vdsm/rest/BindingREST.py
Line 24: def __init__(self, cif, log, params):
wouldn't it be more pythonic to have two optional args instead of this
'params' dict?
....................................................
File vdsm/rest/Controller.py
Line 52: templatePath = "%s/rest/templates" % constants.P_VDSM
Saggi would ask you to put this default up in clientIF and not pollute your new module
with vdsm.constants; I may just beat him to it.
....................................................
File vdsm/rest/Dispatcher.py
Line 1: import cherrypy
GPL boilerplate required, sorry :-(
Line 37: # Decode any leftover %2F in the virtual_path atoms.
I haven't looked deeply, but what do you mean by "leftover"? Usually, uri
decoding should be done once, and completely - if %25 has been decoded already, you may be
decoding a literal %2F, not a slash.
re-implementing decode has plenty of pitfalls. rfc 1738 says:
(The characters "abcdef" may also be used in hexadecimal encodings.)
....................................................
File vdsm.spec.in
Line 45: Requires: python-cherrypy python-cheetah
Have you considered separating this into another sub-rpm?
I think we wanted to factor out xmlrpc, too?
--
To view, visit
http://gerrit.ovirt.org/2021
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b914f7ad82fee8d9e7e3ce6847ffe4cda374a56
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>