Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: update firewall for all services ......................................................................
Patch Set 18: (13 comments) .................................................... File packaging/setup/ovirt_engine_setup/firewall_manager.py Line 38: Line 39: def __str__(self): Line 40: return self.name Line 41: Line 42: def detected(self): Done Line 43: return False Line 44: Line 45: def active(self): Line 46: return False Line 45: def active(self): Line 46: return False Line 47: Line 48: def enable(self): Line 49: return False Done Line 50: Line 51: def print_usage(self): Line 52: return False Line 53: Line 47: Line 48: def enable(self): Line 49: return False Line 50: Line 51: def print_usage(self): Done Line 52: return False Line 53: Line 54: Line 48: def enable(self): Line 49: return False Line 50: Line 51: def print_usage(self): Line 52: return False Done Line 53: Line 54: .................................................... File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py Line 82 Line 83 Line 84 Line 85 Line 86 "this" - _parseFirewalld? Because your comment is 40 lines after the start of this function, and is below _createIptablesConfig . Do we really want now to do this? I personally do not think so. Abstracting this will be done by choosing some more abstract data structure, perhaps a 3-item tuple (name, port, description) or a dict or whatever. I do not think parsing xml files is the right way to go. And I think that it might make sense to do this only during the addition of another manager, which will not happen now. Line 91 Line 92 Line 93 Line 94 Line 95 But still keep the two questions? And also keep 'skip' as the firewall manager? I think it's a bit weird, perhaps even misleading. I see the coolness... Line 224 Line 225 Line 226 Line 227 Line 228 As said above, I'd rather postpone this. Line 101: self._enabled = False Line 102: self._detected_managers = [] Line 103: self._available_managers = [] Line 104: Line 105: class _IpTablesManager(firewall_manager.FirewallManagerBase): Not sure what you mean in 'public' - I removed the 'export' per your request. I really do not have time for this now. We have lots of 'firewall manager' in our code, also in users' answer files. Do you want to rename everything? And cause confusion? Line 106: Line 107: _SERVICE = 'iptables' Line 108: Line 109: def __init__(self, plugin): Line 259: osetupcons.ConfigEnv.FIREWALL_MANAGERS Line 260: ]: Line 261: if manager.detected(): Line 262: self._detected_managers.append(manager) Line 263: We already discussed this in a previous comment. I did this for the time being until you make up your mind. If you want us to alert/err when people put in their answer file: update_manager=True firewall_manager=nonexistantmanager valid_managers=noneistantmanager and if you want this at stage validation, then we need to keep at least until then the input (answer file) value of update_manager and do not overwrite it. Line 264: if ( Line 265: self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] is None and Line 266: self._detected_managers Line 267: ): Line 345: 'Firewall manager to configure ' Line 346: '(@VALUES@) [@DEFAULT@]: ' Line 347: ), Line 348: prompt=True, Line 349: validValues=[m.name for m in self._available_managers], Indeed, thanks. Nice :-) Line 350: caseSensitive=False, Line 351: default=self._available_managers[0].name, Line 352: ) Line 353: firewall_manager = None Line 347: ), Line 348: prompt=True, Line 349: validValues=[m.name for m in self._available_managers], Line 350: caseSensitive=False, Line 351: default=self._available_managers[0].name, That's the current behavior, for quite a long time now (but not in 3.2 IIRC), which I personally find reasonable. Do you really want to change it? between 3.3.1 and 3.3.2? Line 352: ) Line 353: firewall_manager = None Line 354: for manager in self._available_managers: Line 355: if manager.name == response: Line 353: firewall_manager = None Line 354: for manager in self._available_managers: Line 355: if manager.name == response: Line 356: firewall_manager = manager Line 357: break It's very nice indeed, but longer, in number of lines :-(, with our indentation/pep8 rules. Changed anyway. Line 358: Line 359: self.environment[ Line 360: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 361: ] = manager.name Line 358: Line 359: self.environment[ Line 360: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 361: ] = manager.name Line 362: manager.enable() No. It's not really enabling, it's just marking it to be enabled later. otopi needs this earlier than misc - it's checked at validation. Can be low customization or high validation if you want. You are welcome to suggest a better name if you find it confusing. Line 363: Line 364: @plugin.event( Line 365: stage=plugin.Stages.STAGE_VALIDATION, Line 366: condition=lambda self: self._enabled, -- 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: 18 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
