Alon Bar-Lev has posted comments on this change. Change subject: packaging: setup: update firewall for all services ......................................................................
Patch Set 18: (7 comments) please do not submit new patches if you have questions and open issues, until all is resolved. .................................................... File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py Line 82 Line 83 Line 84 Line 85 Line 86 this is what we currently have (parsing xml), while we reorg the source to object oriented form we need to move all related implementation into the designated objects for consistency. Line 91 Line 92 Line 93 Line 94 Line 95 yes, we keep both. if you think that 'skip' is bad... we can add attribute of hidden to remove from selection... Line 224 Line 225 Line 226 Line 227 Line 228 I think we should do this now. Line 101: self._enabled = False Line 102: self._detected_managers = [] Line 103: self._available_managers = [] Line 104: Line 105: class _IpTablesManager(firewall_manager.FirewallManagerBase): public aka you put it externally for other to use. hmmm... what do you mean removed export? oh... now I can see... no... the @export is a must for a class within a module... what I requested is to move the entire class here or to split these into own plugins. 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: this because you keep submitting new patches without finish the discussion. but I still do not understand... self._enabled will be false if nothing was detected, and validation will not be executed, right? so what is the difference? Line 264: if ( Line 265: self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] is None and Line 266: self._detected_managers Line 267: ): 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, yes, while we at, please enforce selection. Line 352: ) Line 353: firewall_manager = None Line 354: for manager in self._available_managers: Line 355: if manager.name == response: Line 358: Line 359: self.environment[ Line 360: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 361: ] = manager.name Line 362: manager.enable() oh... ok.... enable is misleading in this context. I cannot find other term.... but shouldn't this done in validation? after we know that all is ok? 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 <d...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
