Giuseppe Vallarelli has posted comments on this change. Change subject: WIP: Multiple Gateways Feature ......................................................................
Patch Set 10: I would prefer that you didn't submit this (2 inline comments) Design can be further improved, by reading the list of methods is clear that you can have a static or a dynamic source route with related operations, this will make classes more cohesive. .................................................... 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=''): Can type be an empty string? My understanding is that it should be one among route and rule. 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: This static or dynamic configuration question is common also in another patch, I wonder if it would make sense to make this explicit, looks like an important aspect from a domain perspective, instead of a simple boolean they should be 2 different types routes and rules are build in a completely different way this difference can be seen in other different methods. Basically you have almost 2 methods per action one for static configuration and one for dynamic configuration. 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 <amul...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com> Gerrit-Reviewer: Livnat Peer <lp...@redhat.com> Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches