Assaf Muller has posted comments on this change. Change subject: WIP: Multiple Gateways Feature ......................................................................
Patch Set 10: (2 inline comments) Replied to Guiseppe's comments. Thanks for the review Guiseppe! .................................................... File vdsm/sourceRoute.py Line 49: def _buildRules(self): Line 50: return [ipwrapper.Rule(source=self.network, table=self.table), Line 51: ipwrapper.Rule(destination=self.network, table=self.table)] Line 52: Line 53: def _writeCommands(self, commands, type=''): The idea was to improve readability when calling the method. An empty string is not valid input. I'll remove the default value for type and just keep the method call as is. Line 54: filePath = os.path.join(netinfo.NET_CONF_DIR, Line 55: '%s-%s' % (type, self.device)) Line 56: with open(filePath, 'w') as file: Line 57: for command in commands: Line 90: Line 91: self.routes = self._buildRoutes() Line 92: self.rules = self._buildRules() Line 93: Line 94: if self.isStatic: That's a very good point. I've given this a fair amount of thought and talked to Dan about this as well - Should the SourceRoute class be split up to two, or maybe even four classes? (ConfigureStatic, ConfigureDHCP, RemoveStatic, RemoveDHCP), or Static and DHCP, or keep it a single class as is. One thing to keep in mind is there's no real conceptual difference here between a class, or a module with a bunch of methods. You're not going to create more than one instance of "SourceRoute" and its class variables only exist in order to pass less parameters from one function to another. Finally - Splitting up to two / four classes might improve readability or actually hinder it. Right now, SourceRoute is a very simple class. It's still easy to digest it fully in its current size. I think that if it grows in size in the future then it will be worth it to split it up, but currently it is actually better to keep it as a single class for simplicity. Line 95: self._addRoutesStatic() Line 96: self._addRulesStatic() Line 97: else: Line 98: self._addRoutesDHCP() -- To view, visit http://gerrit.ovirt.org/15207 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0224d896724b9cdc44215e92f0da0be71fd19038 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Assaf Muller <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Assaf Muller <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Livnat Peer <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
