Nir Soffer has posted comments on this change. Change subject: vdsm.conf: Add drop-in dir ......................................................................
Patch Set 21: (6 comments) https://gerrit.ovirt.org/#/c/58728/21//COMMIT_MSG Commit Message: Line 26: - /usr/lib/vdsm/vdsm.conf.d/ - for vendor drop-in configuration files. Line 27: - /var/run/vdsm/vdsm.conf.d/ - for admin temporary configuration. Line 28: Line 29: Files with a .conf suffix can be placed into any of the Line 30: vdsm.conf.d drop-in directories. Can you move this text to the module docstring? Commit message is not a replacement for documentation. Line 31: Line 32: Change-Id: I3849829aca50b30742e9c860d7c19296d6361015 Line 33: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1279555 https://gerrit.ovirt.org/#/c/58728/21/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 15 Line 16 Line 17 Line 18 Line 19 We need a docstring here explaining how this module works, the semantics of drop-in confs etc. https://gerrit.ovirt.org/#/c/58728/21/tests/config_test.py File tests/config_test.py: Line 100: Line 101: cfg = config.load('vdsm') Line 102: self.assertEqual(cfg.get('test', 'key'), 'runtime') Line 103: Line 104: def test_dropin_override_default(self): This is the same test as test_runtime_dropin, we can remove it. Line 105: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): Line 106: create_dropin( Line 107: runtime_dir, '51_runtime.conf', '[test]\nkey = runtime\n') Line 108: Line 133: create_dropin(admin_dir, '52.conf', '[test]\nnum = 52\n') Line 134: create_dropin(admin_dir, '53.conf', '[test]\nnum = 53\n') Line 135: Line 136: cfg = config.load('vdsm') Line 137: self.assertEqual(cfg.getint('test', 'num'), 53) We can add the same test when each dropin is in different directory. Will make it clear that the directory does not matter, only the file names. Or use different directories in this test. Line 138: Line 139: def test_not_deleted(self): Line 140: """ Line 141: make sure unrelated items were not modified by reading default Line 135: Line 136: cfg = config.load('vdsm') Line 137: self.assertEqual(cfg.getint('test', 'num'), 53) Line 138: Line 139: def test_not_deleted(self): This test that dropin is modifying only the options it defines, need to make the name more clear about that. Line 140: """ Line 141: make sure unrelated items were not modified by reading default Line 142: values, creating new conf files, running config.load and checking Line 143: the options still exist in ConfigParser Line 146: create_dropin( Line 147: runtime_dir, '51_runtime.conf', '[test]\nkey = runtime\n') Line 148: Line 149: cfg = config.load('vdsm') Line 150: self.assertTrue(cfg.has_option('test', 'num')) Better test also the value. -- To view, visit https://gerrit.ovirt.org/58728 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3849829aca50b30742e9c860d7c19296d6361015 Gerrit-PatchSet: 21 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Irit Goihman <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[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/admin/lists/[email protected]
