Alon Bar-Lev has posted comments on this change.
Change subject: packaging: allow importing ISO domain on setup, exports.d
support
......................................................................
Patch Set 5: (5 inline comments)
....................................................
File packaging/fedora/setup/common_utils.py
Line 1629: """
Line 1630: Check if the given path is already exported
Line 1631: """
Line 1632: if not os.path.exists(exportsFilePath):
Line 1633: return False
I don't like return in a middle of function...
Line 1634: with open(exportsFilePath) as exportsFile:
Line 1635: fileContent = exportsFile.readlines()
Line 1636:
Line 1637: for line in fileContent:
Line 1630: Check if the given path is already exported
Line 1631: """
Line 1632: if not os.path.exists(exportsFilePath):
Line 1633: return False
Line 1634: with open(exportsFilePath) as exportsFile:
Why not:
ret = False
if os.path.exists():
with open() as f:
for line in f:
if verifyString...():
ret = True
break
return ret
Line 1635: fileContent = exportsFile.readlines()
Line 1636:
Line 1637: for line in fileContent:
Line 1638: if verifyStringFormat(line, "^%s\s+.+" % (path)):
....................................................
File packaging/fedora/setup/engine-cleanup.py
Line 176: search_path.append(exportFilePath)
Line 177: msg_files = "%s and %s" % (basedefs.FILE_ETC_EXPORTS,
exportFilePath)
Line 178:
Line 179: if not options.unattended_clean:
Line 180: msg = MSG_CLEAN_NFS_EXPORTS_QUESTION % (basedefs.APP_NAME,
msg_files)
This is not good, as one need to track the message place holders, I suggest use
format:
MSG_CLEAN.format(files=msg_Files)
And inject the APP_NAME into the original message.
Line 181: options.remove_nfs_exports = askYesNo(msg)
Line 182: if not options.remove_nfs_exports:
Line 183: logging.debug("User chose to not clean NFS exports")
Line 184: return
....................................................
File packaging/fedora/setup/engine-setup.py
Line 1742: # Migrate from FILE_ETC_EXPORTS to DIR_ETC_EXPORTSD if
available
Line 1743: exportFilePath = basedefs.FILE_ETC_EXPORTS
Line 1744: if os.path.exists(basedefs.DIR_ETC_EXPORTSD):
Line 1745: exportFilePath = os.path.join(basedefs.DIR_ETC_EXPORTSD,
Line 1746: "%s.exports" % basedefs.ENGINE_SERVICE_NAME)
Maybe give some meaning such as ovirt-engine-iso-domain.exports?
Line 1747:
Line 1748: if utils.isPathInExportFs(controller.CONF["NFS_MP"],
Line 1749: basedefs.FILE_ETC_EXPORTS):
Line 1750: nfsutils.migrateConfig(controller.CONF["NFS_MP"])
Line 1769: utils.copyFile(basedefs.FILE_NFS_SYSCONFIG,
"%s/nfs"%(basedefs.DIR_ETC_SYSCONFIG))
Line 1770:
Line 1771: # Start services
Line 1772: _startNfsServices()
Line 1773: controller.CONF["sd_uuid"] = None
I don't understand this change but I suspect it should be in different
patchset...
Line 1774: for entry in os.listdir(controller.CONF["NFS_MP"]):
Line 1775: path = os.path.join(controller.CONF["NFS_MP"], entry)
Line 1776: if os.path.isdir(path):
Line 1777: try:
--
To view, visit http://gerrit.ovirt.org/11556
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a4343f8374e6439b0ee0a14fff233a0d4a0d5a3
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
