Alon Bar-Lev has posted comments on this change.
Change subject: packaging: setup: update firewall for all services
......................................................................
Patch Set 22:
(13 comments)
OK, as far as firewall manager is concerned I am starting to understand it, I
like the functional code reduce from this module.
As for the introduction of services, this I think we can defer to 3.4 if any,
we need to discuss that, I am unsure there is a great benefit of doing so.
Few minor comments.
Thanks!
....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py
Line 42
Line 43
Line 44
Line 45
Line 46
per my other comments, for now, make this function @staticmethod and pass the
environment as parameter, you can then use it from the other plugins.
the change of using non template non firewalld is quite risky and I would like
to discuss the benefit of doing that, as the format is easy to parse and port
to other technologies as well.
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
for boolean there is either (b) or (not b)
Line 75: )
Line 76: )
Line 77:
Line 78: @plugin.event(
Line 96: if (
Line 97: manager.name in valid_managers or
Line 98: not manager.selectable()
Line 99: ):
Line 100: filtered_managers.append(manager)
remove this condition and add before the loop:
self.environment[osetupcons.ConfigEnv.FIREWALL_MANAGERS] = [
m for m in valid_managers
if not m.selectable() or manager.name in valid_managers
]
also 'filtered_managers' is misleading it is not manager to filter... I would
have called it plain managers... but you do not need this temp variable as far
as I can see.
but for your question...
for x in list[:]:
xxx
this cause list to be copied
Line 101: self.environment[
Line 102: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 103: ] = filtered_managers
Line 104:
Line 120: self.logger.debug(
Line 121: '_customization_is_requested - manager %s' % (
Line 122: manager
Line 123: )
Line 124: )
we have the environment before calling stages, no need to print.
Line 125: if manager.selectable() and manager.detect():
Line 126: self._detected_managers.append(manager)
Line 127:
Line 128: if (
Line 122: manager
Line 123: )
Line 124: )
Line 125: if manager.selectable() and manager.detect():
Line 126: self._detected_managers.append(manager)
?
self._detected_managers = [
manager for manager in ...
if manager.selectable() and manager.detect()
]
Line 127:
Line 128: if (
Line 129: self.environment[
Line 130: osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED
Line 139: 'overwrite current settings.\n'
Line 140: ),
Line 141: )
Line 142: self._enabled = self.environment[
Line 143: osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED
I was curios until now why this is called AUTOMATIC... it is just
UPDATE_FIREWALL
you should find a different name for whatever is used internally.
Line 144: ] = dialog.queryBoolean(
Line 145: dialog=self.dialog,
Line 146: name='OVESETUP_UPDATE_FIREWALL',
Line 147: note=_(
Line 168: def _customization(self):
Line 169: active_managers = []
Line 170: for manager in self._detected_managers:
Line 171: if manager.active():
Line 172: active_managers.append(manager)
?
active_managers = [manager for manager in self._detected_managers if
manager.active()]
Line 173:
Line 174: self._available_managers = (
Line 175: active_managers if active_managers
Line 176: else self._detected_managers
Line 230: def _process_templates(self):
Line 231: for manager in self.environment[
Line 232: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 233: ]:
Line 234: manager.prepare()
I am fine with this one.
Line 235:
Line 236: if self._enabled:
Line 237: if self.environment[
Line 238: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 232: osetupcons.ConfigEnv.FIREWALL_MANAGERS
Line 233: ]:
Line 234: manager.prepare()
Line 235:
Line 236: if self._enabled:
as far as I understand the validation can be with condition=lambda self:
self._enabled, if not please explain why not.
oh... as you need to prepare the examples!!! so split the validation into two
validations... but then I think you can prepare at misc, no?
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(
Line 250: m for m in self._available_managers
Line 251: if m.name == self.environment[
Line 252: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 253: ]
Line 254: ).enable()
right, I just wanted to show you the option.
Line 255:
Line 256: @plugin.event(
Line 257: stage=plugin.Stages.STAGE_CLOSEUP,
Line 258: before=(
....................................................
File
packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager_firewalld.py
Line 78: ),
Line 79: footer_format=(
Line 80: '</service>\n'
Line 81: ),
Line 82: )
I do not like this, we use templates for that, the templates are better as they
can contain complex services.
as I think about it, this change could really be done in different patch and is
not related to this one.
please leave the generation of rules as is.
Line 83:
Line 84: self.environment[
Line 85: otopicons.NetEnv.FIREWALLD_SERVICE_PREFIX +
Line 86: service.name
....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_services.py
Line 118: except IndexError:
Line 119: self.index = 0
Line 120: raise StopIteration
Line 121: self.index += 1
Line 122: return result
why isn't this just a list? what is special?
Line 123:
Line 124: @util.export
Line 125: class Plugin(plugin.PluginBase):
Line 126: """
Line 152: )
Line 153: def _customization(self):
Line 154: # Populates FIREWALL_SERVICES from FIREWALLD_SERVICES.
Line 155: # TODO: Make the various plugins append to FIREWALL_SERVICES
Line 156: # directly and remove this function.
so this patch should go with the the above change, and until resort use the old
method.
however, I am quite unsure that I prefer this method over taking the firewalld
as superset and part it as its format is fairly trivial and can be ported.
so although the work of the other elements solved issues that we actually have,
this one is not really an issue.
so please separate it into different patch and we consider of doing that in 3.4.
Line 157: for firewalld_service in self.environment[
Line 158: osetupcons.NetEnv.FIREWALLD_SERVICES
Line 159: ]:
Line 160: content = osetuputil.processTemplate(
--
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 <[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
