Alon Bar-Lev has posted comments on this change.

Change subject: packaging: setup: update firewall for all services
......................................................................


Patch Set 13:

(3 comments)

....................................................
File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py
Line 162:                 self._detected_managers.append(manager)
Line 163: 
Line 164:         if (osetupcons.Const.FIREWALL_MANAGER_FIREWALLD in
Line 165:             self._detected_managers and
Line 166:             not self.environment[otopicons.NetEnv.FIREWALLD_AVAILABLE]
each manager should have its own detection algorithm, please do not relay on 
service names as general test, it is only available for iptables as firewalld 
is already reported by otopi.

so if you want to do this right, without 'hard coded' tests, as there actually 
there anyway... but having the following will enable it to be constraint.

 class _Manager(object):
     def __init__(self, plugin):
         self._plugin = plugin

     @property
     def name(self):
         raise RuntimeError('Unset')

     def detect(self):
         return False

     def active(self):
         return False

 class _IpTables(_Manager):

     _SERVICE = 'iptables'

     def __init__(self, plugin):
         super(IpTables, self).__init__(plugin)

     @property
     def name(self):
         return 'iptables'

     detect(self):
         return self._plugin.services.exists(self._SERVICE)

     active(self):
         return self._plugin.services.status(self._SERVICE)

 class _Firewalld(_Manager):
     def __init__(self, plugin):
         super(IpTables, self).__init__(plugin)

     @property
     def name(self):
         return 'iptables'

     detect(self):
         return self._plugin.environment(....)

     active(self):
         return self._plugin.services.status(self._SERVICE)

 _MANAGERS = [
     _IpTables,
     _Firewalld,
 ]

Now use _MANAGERS to perform the detection and activation.
Line 167:         ):
Line 168:             self._detected_managers.remove(
Line 169:                 osetupcons.Const.FIREWALL_MANAGER_FIREWALLD
Line 170:             )


Line 170:             )
Line 171: 
Line 172:         if (
Line 173:             self.environment[osetupcons.ConfigEnv.UPDATE_FIREWALL] is 
None and
Line 174:             len(self._detected_managers) > 0
no point to ask if no manager, if no manager is as if no update of firewall, 
this style prevents conditionals in code.
Line 175:         ):
Line 176:             self.dialog.note(
Line 177:                 text=_(
Line 178:                     'Setup can automatically configure the firewall '


Line 258:                             osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 259:                         ] = manager
Line 260:                         break
Line 261: 
Line 262:         self.environment[otopicons.NetEnv.IPTABLES_ENABLE] = (
If we do no set firewall, we do not care what manager is set.

If we are to set firewall then the manager must meet something we actually can 
work with.

Please give examples of where we ignore invalid settings when actually using 
them.
Line 263:             self.environment[
Line 264:                 osetupcons.ConfigEnv.FIREWALL_MANAGER
Line 265:             ] == osetupcons.Const.FIREWALL_MANAGER_IPTABLES
Line 266:         )


-- 
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: 13
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

Reply via email to