Simone Tiraboschi has posted comments on this change. Change subject: packaging: setup: Adding a dialog to let the user review iptables changes ......................................................................
Patch Set 1: (7 comments) http://gerrit.ovirt.org/#/c/33085/1/packaging/setup/ovirt_engine_setup/constants.py File packaging/setup/ovirt_engine_setup/constants.py: Line 380: def UPDATE_FIREWALL(self): Line 381: return 'OVESETUP_CONFIG/updateFirewall' Line 382: Line 383: FIREWALL_MANAGERS = 'OVESETUP_CONFIG/firewallManagers' Line 384: SKIP_FIREWALL_REVIEW = 'OVESETUP_CONFIG/skipFirewallReview' > I'd call it something like IPTABLES_CHANGES_CONFIRMED The changes we are going to apply to iptables configuration depend not only from our code but also from initial external configuration. So on different system, with the same answer file, we can get different results due to different initial configurations so I prefer to just add a env variable to skip at all the review process instead of saving the response to a question that changes between different system. For the same reason I'm not really sure about answer file: in the setup process I'm just asking if the user approves the modification so I'm not really asking if he want to skip the review process. for unattended setup is enough to manually add this key to the answer file in order to skip at all the review process. Line 385: VALID_FIREWALL_MANAGERS = 'OVESETUP_CONFIG/validFirewallManagers' Line 386: FQDN_REVERSE_VALIDATION = 'OVESETUP_CONFIG/fqdnReverseValidation' Line 387: FQDN_NON_LOOPBACK_VALIDATION = 'OVESETUP_CONFIG/fqdnNonLoopback' Line 388: http://gerrit.ovirt.org/#/c/33085/1/packaging/setup/plugins/ovirt-engine-common/base/network/firewall_manager_iptables.py File packaging/setup/plugins/ovirt-engine-common/base/network/firewall_manager_iptables.py: Line 51: class _IpTablesManager(firewall_manager_base.FirewallManagerBase): Line 52: Line 53: _SERVICE = 'iptables' Line 54: Line 55: _REDHAT_IPTABLES = '/etc/sysconfig/iptables' > You already have SYSCONFIG_IPTABLES Done Line 56: Line 57: def _get_rules(self): Line 58: if self._rules is None: Line 59: self._rules = outil.processTemplate( Line 138: fromfile=_('current'), Line 139: tofile=_('proposed'), Line 140: ) Line 141: for line in diff: Line 142: diffl += line > Perhaps: Done Line 143: if len(diffl) > 0: Line 144: confirmed = dialog.queryBoolean( Line 145: dialog=self.plugin.dialog, Line 146: name='OVESETUP_RPMDISTRO_REQUIRE_ROLLBACK', Line 142: diffl += line Line 143: if len(diffl) > 0: Line 144: confirmed = dialog.queryBoolean( Line 145: dialog=self.plugin.dialog, Line 146: name='OVESETUP_RPMDISTRO_REQUIRE_ROLLBACK', > Change the name after copying :-) Done Line 147: note=_( Line 148: 'Generated iptables rules diverge from current ones.\n' Line 149: 'Please review the changes:\n\n' Line 150: '{diff}\n\n' Line 144: confirmed = dialog.queryBoolean( Line 145: dialog=self.plugin.dialog, Line 146: name='OVESETUP_RPMDISTRO_REQUIRE_ROLLBACK', Line 147: note=_( Line 148: 'Generated iptables rules diverge from current ones.\n' > diverge? I am not a native English speaker but it sounds a bit weird. I'd j Done Line 149: 'Please review the changes:\n\n' Line 150: '{diff}\n\n' Line 151: 'Do you want to proceed with firewall configuration? ' Line 152: '(@VALUES@) [@DEFAULT@]: ' http://gerrit.ovirt.org/#/c/33085/1/packaging/setup/plugins/ovirt-engine-setup/base/network/firewall_manager.py File packaging/setup/plugins/ovirt-engine-setup/base/network/firewall_manager.py: Line 238: ), Line 239: ) Line 240: def _review_config(self): Line 241: if not self.environment[ Line 242: osetupcons.ConfigEnv.SKIP_FIREWALL_REVIEW > I'd not check it here but only in iptables plugin Here we are under plugins/ovirt-engine-setup/base/network/ so it's executed just on on engine-setup. We don't have packaging/setup/plugins/ovirt-engine-setup/base/network/firewall_manager_iptables.py while we have packaging/setup/plugins/ovirt-engine-common/base/network/firewall_manager_iptables.py so it should be called from here to be sure it will be called only on engine-setup It' a kind of polymorphic approach on an abstract firewall manager, only iptables one really implements it. Line 243: ]: Line 244: for manager in self.environment[ Line 245: osetupcons.ConfigEnv.FIREWALL_MANAGERS Line 246: ]: Line 242: osetupcons.ConfigEnv.SKIP_FIREWALL_REVIEW Line 243: ]: Line 244: for manager in self.environment[ Line 245: osetupcons.ConfigEnv.FIREWALL_MANAGERS Line 246: ]: > All of them? Why not only self.environment[osetupcons.ConfigEnv.FIREWALL_MA Done Line 247: manager.review_config() Line 248: Line 249: @plugin.event( Line 250: stage=plugin.Stages.STAGE_MISC, -- To view, visit http://gerrit.ovirt.org/33085 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63e0eeb26d925c8c79b9c8e55da64c57ce94a3f6 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
