Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: update firewall for all services ......................................................................
Patch Set 13: (4 comments) .................................................... File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py Line 162: self._detected_managers.append(manager) Line 163: Line 164: if (osetupcons.Const.FIREWALL_MANAGER_FIREWALLD in Line 165: self._detected_managers and Line 166: not self.environment[otopicons.NetEnv.FIREWALLD_AVAILABLE] > there is no relationship between 'service' and firewall manager, the > detection can be using service but nobody promise you that: > 1. there is service. > 2. the service name equals to the manager name. Currently it is so. This does allow someone to add a plugin for another, service-based firewall manager, without touching this code. If we hardcode tests as you suggest, we get longer code, which does the same, plus prevents adding plugins. Why is it better? Line 167: ): Line 168: self._detected_managers.remove( Line 169: osetupcons.Const.FIREWALL_MANAGER_FIREWALLD Line 170: ) Line 170: ) Line 171: Line 172: if ( Line 173: self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] is None and Line 174: len(self._detected_managers) > 0 > so why do you submit new patch? I did apply part of your comments. Pushing allows you to see if I got you right. I somehow was under the impression there is no rule against this. > if not self._detected_managers: > self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] = False > if self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] is None: > ask user I thought this might be what you meant but I do not see how it helps us. As is, it asks the user even if there are no managers. If you want to prevent that, you get even more complex logic. And there are no "future conditions". Or I am missing something. Line 175: ): Line 176: self.dialog.note( Line 177: text=_( Line 178: 'Setup can automatically configure the firewall ' Line 210: def _customization(self): Line 211: active_managers = [] Line 212: for manager in self._detected_managers: Line 213: if self.services.status(manager): Line 214: active_managers.append(manager) > you do not need to check for 'active' at _customization_is_enabled, you need > to check if there is at least one supported firewall. That's what I do. > the active check belongs to this phase, and should not be based of services. Belongs to which phase? How should it be checked if not services? Line 215: Line 216: available_managers = ( Line 217: active_managers if active_managers Line 218: else self._detected_managers Line 258: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 259: ] = manager Line 260: break Line 261: Line 262: self.environment[otopicons.NetEnv.IPTABLES_ENABLE] = ( > simply do nothing is not good, as there was explicit request. There are many such cases. The way the code works, allows to pass in env values which do not make sense. Do you think we should abort here? Warn? > I thought that you want to ask for firewall update every upgrade, so I do not > understand the above comment. How do these contradict? Line 263: self.environment[ Line 264: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 265: ] == osetupcons.Const.FIREWALL_MANAGER_IPTABLES Line 266: ) -- To view, visit http://gerrit.ovirt.org/20737 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If3c1a634b2e8539ebd604205b5487290c8d8a1a9 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ofer Schreiber <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
