Dan Kenigsberg has posted comments on this change. Change subject: net: introducing KernelConfig ......................................................................
Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/41973/4/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 330: self.assertIn(bondName, netinfo.bondings) Line 331: self.assertEqual(netinfo.bondings[bondName]['active_slave'], '') Line 332: Line 333: def setupNetworks(self, *args, **kwargs): Line 334: status, msg = self.vdsm_net.setupNetworks(*args, **kwargs) > I disagree. IMO it is 'counter-noise' since self.vdsm_net.setupNetworks is tests/functional/utils's VdsProxy is intended as a one-stop object accessing the running Vdsm, that is used by all functional tests (yes, unfortunately not so many functional tests other than the nework ones are regularly run) A setupNetworks already exists there, and your new assertion would fit there perfectly. I can see why you dislike the self.vdsm_net boilerplate. I would not mind replacing it with a module-level (or a class-level, like you propose) shorthand, but please - let us keep this change to its own patch. it changes so many lines and obscures the real change, which is relatively small. Line 335: self._assert_kernel_config_matches_running_config() Line 336: return status, msg Line 337: Line 338: def _assert_kernel_config_matches_running_config(self): -- To view, visit https://gerrit.ovirt.org/41973 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79f1eb553a42f1398ad12aa1bc33522f8af30c79 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
