Alon Bar-Lev has posted comments on this change.
Change subject: packaging: setup: update firewall for all services
......................................................................
Patch Set 16:
(8 comments)
....................................................
File packaging/setup/ovirt_engine_setup/firewall_manager_base.py
name of module should not have _base suffix even if now it has only one class.
Line 1: #
Line 2: # ovirt-engine-setup -- ovirt engine setup
Line 3: # Copyright (C) 2013 Red Hat, Inc.
Line 4: #
Line 27:
Line 28: @property
Line 29: def plugin(self):
Line 30: return self._plugin
Line 31:
I think that if you add:
def __str__(self):
return self.name
you see this objects nicely in environment dump.
Line 32: @property
Line 33: def environment(self):
Line 34: return self._plugin.environment
Line 35:
....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py
Line 197: x.strip()
Line 198: for x in self.environment[
Line 199: osetupcons.ConfigEnv.SUPPORTED_FIREWALL_MANAGERS
Line 200: ].split(',')
Line 201: ]
once again these are not supported, what supported is what product supports.
this is filtering of what supported into something that downstream want to
enable.
Line 202:
Line 203: for manager in self.environment[
Line 204: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 205: ]:
Line 211: detected,
Line 212: supported,
Line 213: ),
Line 214: )
Line 215: if supported and detected:
why not just add the debug message here?
'detected xxx', we already see in debug the env values of what detected and
supported.
if you have __str__ on modules we see the environment dump anyway... so no need
to add debug messages at all.
Line 216: self._detected_managers.append(manager)
Line 217:
Line 218: if not self._detected_managers:
Line 219: self._enabled = self.environment[
Line 304: default=self._available_managers[0].name,
Line 305: )
Line 306: self.environment[
Line 307: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 308: ] = response
please store the object and not the string.
Line 309:
Line 310: self.environment[otopicons.NetEnv.IPTABLES_ENABLE] = (
Line 311: self.environment[
Line 312: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 309:
Line 310: self.environment[otopicons.NetEnv.IPTABLES_ENABLE] = (
Line 311: self.environment[
Line 312: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 313: ] == osetupcons.Const.FIREWALL_MANAGER_IPTABLES
please add to interface.
manager.enable()
Line 314: )
Line 315: self.environment[otopicons.NetEnv.FIREWALLD_ENABLE] = (
Line 316: self.environment[
Line 317: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 333: manager=self.environment[
Line 334: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 335: ],
Line 336: ),
Line 337: )
no need for exception and log, the exception will be logged.
Line 338: raise RuntimeError(
Line 339: _(
Line 340: 'Firewall manager {manager} is not available'
Line 341: ).format(
Line 487: commands='\n'.join([
Line 488: ' ' + l
Line 489: for l in commands
Line 490: ]),
Line 491: )
please move this into interface as well, it will be much more coherent even if
values are hardcoded for now.
Line 492: )
Line 493:
Line 494:
--
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: 16
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
