Nir Soffer has posted comments on this change. Change subject: vdsm.conf: Add drop-in dir ......................................................................
Patch Set 6: Code-Review-1 (5 comments) Needs tests, remove stuff vdsm does not need. https://gerrit.ovirt.org/#/c/48317/6/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 21: from __future__ import absolute_import Line 22: import os Line 23: from six.moves import configparser Line 24: import textwrap Line 25: from glob import glob Do not import functions from module, only modules. Line 26: Line 27: parameters = [ Line 28: # Section: [vars] Line 29: ('vars', [ Line 464: Line 465: config = configparser.ConfigParser() Line 466: set_defaults(config) Line 467: Line 468: def read_configs(pkgname, paths=["/usr/lib/", "/run/", "/etc/"]): Why check configuration in /usr/lib or /run? it doe not make sense. I don't see a need for anything except /etc/. You are adding too generic function we don't need. Please remove the paths we don't need, or explain why we need them in the commit message. Line 469: """This function is reading config files in a specific scheme Line 470: Line 471: The function reads - for one component - config files from several Line 472: locations and in addition it is also reading configuration snippets Line 478: configuration file into /etc or a drop-in directory. Line 479: """ Line 480: for path in paths: Line 481: conffile = os.path.join(path, pkgname + ".conf") Line 482: confdropin = conffile + ".d" Not needed here, create it just before we use it. Also, the name should be conf_dropin_dir. Line 483: Line 484: if os.path.exists(conffile): Line 485: config.read([conffile]) Line 486: Line 483: Line 484: if os.path.exists(conffile): Line 485: config.read([conffile]) Line 486: Line 487: if os.path.exists(confdropin): os.path.isdir Line 488: config.read(sorted(glob(confdropin + "/*.conf"))) Line 489: Line 490: read_configs("vdsm") Line 491: Line 484: if os.path.exists(conffile): Line 485: config.read([conffile]) Line 486: Line 487: if os.path.exists(confdropin): Line 488: config.read(sorted(glob(confdropin + "/*.conf"))) You are doing too much on one line. Please do this like this: pattern = os.path.join(conf_dropin_dir, "*.conf") config.read(sorted(glob.glob(pattern)) Line 489: Line 490: read_configs("vdsm") Line 491: Line 492: if __name__ == '__main__': -- To view, visit https://gerrit.ovirt.org/48317 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I707a1865e8d60dc4dcdc0e681b52c07c75f1c409 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Fabian Deutsch <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
