Alon Bar-Lev has posted comments on this change.
Change subject: packaging: setup: update firewall for all services
......................................................................
Patch Set 12:
(6 comments)
....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py
Line 198: prompt=True,
Line 199: true=_('Yes'),
Line 200: false=_('No'),
Line 201: default=True,
Line 202: )
add:
if (
self.environment[osetupcons.ConfigEnv.FIREWALL_MANAGER] is None and
len(available_managers) == 1
):
self.environment[osetupcons.ConfigEnv.FIREWALL_MANAGER] =
available_managers[0]
but here you see the complexity, should condition be on active managers or
available manager? how do we cope with multiple active manager? why do we care
about the available once we find several active?
the term available and active are confusing.
we use whatever active - how can be more than one?
if no active we use what available.
from this point forward there should be single variable.
algorithm should be:
1. input is 'supported' set, selected firewall (optional)
2. do we want to update firewall? if not just skip, this can be in own stage,
no need to mix selection and enable
3. detected = [whatever detected] as subset of supported
4. we can print a nice user message at this point of detected status if we
want, not that important.
5. available = whatever active in detected
6. if available empty available is detected
7. if not selected and len(detected) == 1 selected=detected[0]
8. if not selected ask user out of available
9. now you have whatever selected
side note... not sure I understand how it can be more than one active.
Line 203:
Line 204: if (
Line 205: self.environment[osetupcons.ConfigEnv.FIREWALL_MANAGER]
is None and
Line 206: self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL]
Line 204: if (
Line 205: self.environment[osetupcons.ConfigEnv.FIREWALL_MANAGER]
is None and
Line 206: self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL]
Line 207: ):
Line 208: if len(available_managers) > 1:
remove this condition
Line 209: self.dialog.note(
Line 210: text=_(
Line 211: 'The following firewall managers were
detected on '
Line 212: 'this system: {managers}\n'
Line 245: if response.lower() == manager.lower():
Line 246: self.environment[
Line 247: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 248: ] = manager
Line 249: break
I do not understand... only valid values can be gotten out of the query, why do
you need to loop?
The keys/constants we hold can be lower so there should be no problem in
getting these as-is.
Line 250: elif len(available_managers) == 1:
Line 251: self.environment[
Line 252: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 253: ] = available_managers[0]
Line 246: self.environment[
Line 247: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 248: ] = manager
Line 249: break
Line 250: elif len(available_managers) == 1:
this can be autoselect before the condition, regardless, please put the shorter
block as first in if statement.
Line 251: self.environment[
Line 252: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 253: ] = available_managers[0]
Line 254: self.dialog.note(
Line 259: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 260: ],
Line 261: ),
Line 262: )
Line 263:
here you should check that whatever selected (via user or answer) is valid out
of active.
Line 264: if self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL]:
Line 265: self.environment[otopicons.NetEnv.IPTABLES_ENABLE] = (
Line 266: self.environment[
Line 267: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 270: self.environment[otopicons.NetEnv.FIREWALLD_ENABLE] = (
Line 271: self.environment[
Line 272: osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 273: ] == osetupcons.Const.FIREWALL_MANAGER_FIREWALLD
Line 274: )
I suggest to touch only if support should be applied.
Line 275:
Line 276: @plugin.event(
Line 277: stage=plugin.Stages.STAGE_VALIDATION,
Line 278: name=osetupcons.Stages.NET_FIREWALL_MANAGER_PROCESS_TEMPLATES,
--
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: 12
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
