Alon Bar-Lev has posted comments on this change.
Change subject: packaging: setup: update firewall for all services
......................................................................
Patch Set 22:
(4 comments)
....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py
Line 70: self.environment[osetupcons.CoreEnv.DEVELOPER_MODE]
Line 71: or (
Line 72: self.environment[
Line 73:
osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED
Line 74: ] is False
So I prefer "x is not None and not x" and remove the comment... better code
than comments.
oh... and you have not at outside... this is too complex for me...
self._enabled = (
not development and (
update_firewall is not None and
update_firewall
)
)
I am trying to think positive...
Line 75: )
Line 76: )
Line 77:
Line 78: @plugin.event(
Line 139: 'overwrite current settings.\n'
Line 140: ),
Line 141: )
Line 142: self._enabled = self.environment[
Line 143: osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED
I do not understand what automatic and what is requested.... it is plain update
firewall, of course that if set it is requested... so I still think this should
be reverted to UPDATE_FIREWALL as it used to be and if you need something
internally add other variable.... but I cannot see how you use anything
internally right now.... so I am confused.
Line 144: ] = dialog.queryBoolean(
Line 145: dialog=self.dialog,
Line 146: name='OVESETUP_UPDATE_FIREWALL',
Line 147: note=_(
Line 232: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 233: ]:
Line 234: manager.prepare()
Line 235:
Line 236: if self._enabled:
well, the reason I am thinking is that we have a logic that is always happening
an a logic that happens only if we are enabled, these are two separate flows.
the if is just the side effect.
the 'enable()' should put whatever needed in otopi to actually perform the
change, I should be able to remove the 'prepare()' which is just putting the
example and all should work well.
hence these are two unrelated logics.
Line 237: if self.environment[
Line 238: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 239: ] not in [m.name for m in self._available_managers]:
Line 240: raise RuntimeError(
....................................................
File
packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager_human.py
Line 80: text=_(
Line 81: 'The following network ports should be opened:\n'
Line 82: '{ports}'
Line 83: ).format(
Line 84: ports=self._output,
do we need self._output? can't we skip the prepare?
Line 85: ),
Line 86: )
Line 87:
Line 88: @plugin.event(
--
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: 22
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
