Mark Wu has posted comments on this change.
Change subject: move get-conf-item/set-conf-item to vdsm-tool
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File vdsm-tool/get-conf-item.py
Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19:
Line 20: # Access python's config files from bash by vdsm-tool
Provide access to vdsm's configuration file for the command vdsm-tool
Better?
I suggest you use triple quotes for multiple lines comments
Line 21: # Usage: vdsm-tool get-conf-item filename.conf section item default
Line 22:
Line 23: from vdsm.tool import expose
Line 24: try:
Line 29: vdsmconf = ConfigParser.ConfigParser()
Line 30:
Line 31:
Line 32: def _get_conf_item(file, section, item, default):
Line 33: vdsmconf.read(file)
if vdsm.config is imported successfully, we don't need load the file again.
Line 34: try:
Line 35: return vdsmconf.get(section, item)
Line 36: except:
Line 37: return default
Line 37: return default
Line 38:
Line 39:
Line 40: @expose("get-conf-item")
Line 41: def print_conf_item(file, section, item, default):
I don't think we need expose configuration file as a parameter. You can just
use the path of vdsm.conf.
The same to the 'default'. There's a default value in vdsm.config already.
Why do we need the caller to specify it?
Line 42: """
Line 43: get item from config files
Line 44: Usage: get-conf-item filename.conf section item default
Line 45: """
Line 39:
Line 40: @expose("get-conf-item")
Line 41: def print_conf_item(file, section, item, default):
Line 42: """
Line 43: get item from config files
configuration file
Line 44: Usage: get-conf-item filename.conf section item default
Line 45: """
Line 46: val = _get_conf_item(file, section, item, default)
Line 47: if val.lower() in ["false", "true"]:
....................................................
File vdsm-tool/Makefile.am
Line 23: vdsm-tool
Line 24:
Line 25: EXTRA_DIST = \
Line 26: get-conf-item.py \
Line 27: set-conf-item.py \
Why put them to the 'EXTRA_DIST' list? They're already in ist_vdsmtool_DATA
Line 28: validate_ovirt_certs.py.in
Line 29:
Line 30: dist_vdsmtool_DATA = \
Line 31: __init__.py \
....................................................
File vdsm-tool/set-conf-item.py
Line 20: # Set python's config files from the shell by vdsm-tool.
Line 21: # Usage: vdsm-tool set-conf-item filename.conf section item value
Line 22:
Line 23: # WARNING: comments and item order within config file will be destoryed
Line 24:
The comments here also needs an update like the get-conf-item.
I think we should merge get-conf-item and set-conf-time into one file.
Line 25: from vdsm.tool import expose
Line 26: try:
Line 27: from vdsm.config import config
Line 28: vdsmconf = config
--
To view, visit http://gerrit.ovirt.org/7695
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c64de097bbaea6a8cf862b43243377e10e00391
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Bing Bu Cao <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Ryan Harper <[email protected]>
Gerrit-Reviewer: ShaoHe Feng <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: Wenyi Gao <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
