Alon Bar-Lev has posted comments on this change.
Change subject: packaging: setup: update firewall for all services
......................................................................
Patch Set 18:
(11 comments)
ok, much better!
....................................................
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):
this is active detect function not passive property.
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
return false -> pass
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):
I think better term is manual_installation or something similar.
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
return false -> pass
Line 53:
Line 54:
....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py
Line 101: self._enabled = False
Line 102: self._detected_managers = []
Line 103: self._available_managers = []
Line 104:
Line 105: class _IpTablesManager(firewall_manager.FirewallManagerBase):
if FirewallManagerBase is public, please separate these two classes to own
plugins, firewall_provider_iptables and firewall_profider_firewalld.
each should have its own provider class, and I think the FirewallManagerBase
should be FirewallProviderBase.
I also have some thoughts of doing this at common, and move functionality of
packaging/setup/plugins/ovirt-engine-remove/network/firewalld.py into this
provider as well.
Line 106:
Line 107: _SERVICE = 'iptables'
Line 108:
Line 109: def __init__(self, plugin):
Line 240: ]:
Line 241: if manager.name not in valid_managers:
Line 242: self.environment[
Line 243: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 244: ].remove(manager)
nice! above is self explanatory.
Line 245:
Line 246: @plugin.event(
Line 247: stage=plugin.Stages.STAGE_CUSTOMIZATION,
Line 248: condition=lambda self: self._enabled,
Line 259: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 260: ]:
Line 261: if manager.detected():
Line 262: self._detected_managers.append(manager)
Line 263:
if you add:
if not self._detected_managers:
self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] = False
then there is no need to ask twice about the detected_managers, making the
bellow condition simpler.
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],
I think that with the __str__ you can have this list as is, not sure :)
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,
I am unsure we want default if we have multiple value, forcing user to select.
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
I read something nice such:
next(m for m in self._available_managers if m.name == response)
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()
hmmm, do this at misc?
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
