Change in vdsm[master]: gluster: Gluster UFO verbs
barumuga at redhat.com
barumuga at redhat.com
Wed Feb 13 06:20:19 UTC 2013
Bala.FA has posted comments on this change.
Change subject: gluster: Gluster UFO verbs
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(7 inline comments)
....................................................
File vdsm_cli/vdsClientGluster.py
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19:
Line 20: import pprint as pp
Line 21: import json
leave a newline to separate user/system modules
Line 22: from vdsClient import service
Line 23:
Line 24:
Line 25: class GlusterService(service):
....................................................
File vdsm/gluster/api.py
Line 216: status = self.svdsmProxy.glusterVolumeProfileInfo(volumeName, nfs)
Line 217: return {'profileInfo': status}
Line 218:
Line 219: @exportAsVerb
Line 220: def swiftConfigGet(self, serverType, section=None, configOption=None):
please add options=None for future use
Line 221: return self.svdsmProxy.glusterSwiftConfigGet(serverType,
Line 222: section,
Line 223: configOption)
Line 224:
Line 222: section,
Line 223: configOption)
Line 224:
Line 225: @exportAsVerb
Line 226: def swiftConfigSet(self, serverType, configDict):
please add options=None for future use
Line 227: self.svdsmProxy.glusterSwiftConfigSet(serverType,
Line 228: configDict)
Line 229:
Line 230:
....................................................
File vdsm/gluster/cli.py
Line 19: #
Line 20:
Line 21: import xml.etree.cElementTree as etree
Line 22: from functools import wraps
Line 23: import ConfigParser
please add new line to separate system/user modules
Line 24: from vdsm import utils
Line 25: from vdsm import netinfo
Line 26: import exception as ge
Line 27: from hostname import getHostNameFqdn, HostNameException
Line 910: config_file = SWIFT_CONFIG_FILES[serverType]
Line 911: try:
Line 912: config.readfp(open(config_file))
Line 913: except IOError:
Line 914: raise ge.GlusterSwiftConfigOpenFailedException()
please pass actual error from IOError to GlusterSwiftConfigOpenFailedException(). In there, construct self.message appropriately.
Line 915:
Line 916: return config
Line 917:
Line 918:
Line 921: try:
Line 922: with open(config_file, 'wb') as configFile:
Line 923: config.write(configFile)
Line 924: except IOError:
Line 925: raise ge.GlusterSwiftConfigWriteFailedException()
same here
Line 926:
Line 927:
Line 928: @exportToSuperVdsm
Line 929: def swiftConfigGet(serverType, section=None, configOption=None):
....................................................
File vdsm/gluster/exception.py
Line 407: message = "Invalid section"
Line 408:
Line 409: def __init__(self, *args, **kwargs):
Line 410: section = kwargs["sec"] if "sec" in kwargs else ""
Line 411: self.message = "%s: Invalid Section" % section
We need a docstring describing what kwargs are accepted. Moreover I feel kwargs doesn't convey the api usability. I would like positional argumnet instead. But its your call
Line 412:
Line 413:
Line 414: class GlusterSwiftConfigInvalidServerException(GlusterGeneralException):
Line 415: code = 4411
--
To view, visit http://gerrit.ovirt.org/10864
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list