Irit Goihman has posted comments on this change. Change subject: vdsm.conf: Add drop-in dir ......................................................................
Patch Set 18: (14 comments) https://gerrit.ovirt.org/#/c/58728/18/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 537: dropins.sort(key=os.path.basename) Line 538: cfg.read(dropins) Line 539: Line 540: Line 541: config = load('vdsm') > If we have a default, we don't need to use it here. Done Line 542: Line 543: if __name__ == '__main__': https://gerrit.ovirt.org/#/c/58728/18/lib/vdsm/tool/configurators/libvirt.py File lib/vdsm/tool/configurators/libvirt.py: Line 1 Line 2 > The changes in this file are not related to supporting dropins. Please sepa Done https://gerrit.ovirt.org/#/c/58728/18/lib/vdsm/vdscli.py File lib/vdsm/vdscli.py: Line 77 Line 78 Line 79 Line 80 Line 81 > Not sure why the old code was reading the config here when config was jst r Done https://gerrit.ovirt.org/#/c/58728/18/tests/config_test.py File tests/config_test.py: Line 28: ( Line 29: 'test', Line 30: [ Line 31: ('ssl', 'true', None), Line 32: ('num', '0', None), > Why use boolean and integer? using strings we can make sure that each type Done Line 33: ], Line 34: ) Line 35: ] Line 36: Line 36: Line 37: Line 38: @contextmanager Line 39: def fakedirs(): Line 40: with namedTemporaryDir() as path: > path is not specific enough, use something like tmpdir. Done Line 41: admin_dir = os.path.join(path, 'etc/') Line 42: vendor_dir = os.path.join(path, 'usr/lib') Line 43: runtime_dir = os.path.join(path, 'run/') Line 44: paths = (admin_dir, vendor_dir, runtime_dir) Line 37: Line 38: @contextmanager Line 39: def fakedirs(): Line 40: with namedTemporaryDir() as path: Line 41: admin_dir = os.path.join(path, 'etc/') > No need for trailing slash in paths. Done Line 42: vendor_dir = os.path.join(path, 'usr/lib') Line 43: runtime_dir = os.path.join(path, 'run/') Line 44: paths = (admin_dir, vendor_dir, runtime_dir) Line 45: with MonkeyPatchScope( Line 38: @contextmanager Line 39: def fakedirs(): Line 40: with namedTemporaryDir() as path: Line 41: admin_dir = os.path.join(path, 'etc/') Line 42: vendor_dir = os.path.join(path, 'usr/lib') > Use: Done Line 43: runtime_dir = os.path.join(path, 'run/') Line 44: paths = (admin_dir, vendor_dir, runtime_dir) Line 45: with MonkeyPatchScope( Line 46: [(config, '_SYSCONFDIR', admin_dir), Line 39: def fakedirs(): Line 40: with namedTemporaryDir() as path: Line 41: admin_dir = os.path.join(path, 'etc/') Line 42: vendor_dir = os.path.join(path, 'usr/lib') Line 43: runtime_dir = os.path.join(path, 'run/') > Remove trailing slash Done Line 44: paths = (admin_dir, vendor_dir, runtime_dir) Line 45: with MonkeyPatchScope( Line 46: [(config, '_SYSCONFDIR', admin_dir), Line 47: (config, '_DROPPIN_BASES', paths), Line 40: with namedTemporaryDir() as path: Line 41: admin_dir = os.path.join(path, 'etc/') Line 42: vendor_dir = os.path.join(path, 'usr/lib') Line 43: runtime_dir = os.path.join(path, 'run/') Line 44: paths = (admin_dir, vendor_dir, runtime_dir) > paths -> dirs Done Line 45: with MonkeyPatchScope( Line 46: [(config, '_SYSCONFDIR', admin_dir), Line 47: (config, '_DROPPIN_BASES', paths), Line 48: (config, 'parameters', parameters)]): Line 66: with open(path, "w") as f: Line 67: f.write(content) Line 68: Line 69: Line 70: class TestConfig(VdsmTestCase): > Please add simple test for getting the defaults when no conf file exists Done Line 71: def test_default(self): Line 72: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): Line 73: create_conf(admin_dir, '[test]\nssl = false\n') Line 74: Line 91: Line 92: cfg = config.load() Line 93: self.assertFalse(cfg.getboolean('test', 'ssl')) Line 94: Line 95: def test_dropin_override_conf(self): > This repeat test_dropins_priority. Done Line 96: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): Line 97: create_conf(admin_dir, '[test]\nnum = 1\n') Line 98: create_dropin( Line 99: runtime_dir, '51_runtime.conf', '[test]\nnum = 51\n') Line 102: self.assertEqual(cfg.getint('test', 'num'), 51) Line 103: Line 104: def test_ignore_invalid_conf_suffix(self): Line 105: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): Line 106: create_conf(admin_dir, '[test]\nssl = false\n') > We don't need conf here, the defaults are enough for testing that the dropi Done Line 107: create_dropin( Line 108: runtime_dir, '50_invalid_name', '[test]\nssl = true\n') Line 109: Line 110: cfg = config.load() Line 111: self.assertFalse(cfg.getboolean('test', 'ssl')) Line 112: Line 113: def test_same_directory_read_proirity(self): Line 114: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): Line 115: create_conf(admin_dir, '[test]\nnum = 1\n') > conf not needed for this test. Done Line 116: create_dropin(admin_dir, '51.conf', '[test]\nnum = 51\n') Line 117: create_dropin(admin_dir, '52.conf', '[test]\nnum = 52\n') Line 118: create_dropin(admin_dir, '53.conf', '[test]\nnum = 53\n') Line 119: https://gerrit.ovirt.org/#/c/58728/18/tests/toolTests.py File tests/toolTests.py: Line 316: Line 317: with monkeypatch.MonkeyPatchScope( Line 318: [(config, '_SYSCONFDIR', vdsm_path), Line 319: (config, '_DROPPIN_BASES', (vdsm_path,))]): Line 320: self.vdsm_cfg = config.load() > Why do we need to work so hard here? why not just mock libvirt.config (and Done, part of a new patch Line 321: Line 322: self._setConfig( Line 323: ('QLCONF', 'libvirtd'), Line 324: ('LDCONF', 'qemu_sanlock'), -- 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: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
