Nir Soffer has posted comments on this change. Change subject: packaging: setup: fixing iptables review dialog ......................................................................
Patch Set 2: (8 comments) http://gerrit.ovirt.org/#/c/34011/2//COMMIT_MSG Commit Message: Line 6: Line 7: packaging: setup: fixing iptables review dialog Line 8: Line 9: Writing the will to review iptables changes into answer file, Line 10: fixing a bug Reading the title, this should be a bug fix. Looking into the code, there is lot of code improving the review process, and I don't see where is the bug fix. It seems that this patch mix several unrelated changes. Can you explain what it the bug fix? Line 11: Line 12: Change-Id: I8f16a90a074052226882d76fc80918ed276d379c Line 13: Bug-Url: https://bugzilla.redhat.com/1109326 http://gerrit.ovirt.org/#/c/34011/2/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 136: with open( Line 137: osetupcons.FileLocations.SYSCONFIG_IPTABLES, Line 138: 'r' Line 139: ) as current: Line 140: current_rules = current.read().splitlines() This pattern is racy. Instead you can simply open the file, and handle ENOENT error: try: with open(path) as f: rules = f.readlines() except IOError as e: if e.errno != errno.ENOENT: raise rules = [] This also avoid the need to repeat this way too long name: osetupcons.FileLocations.SYSCONFIG_IPTABLES And readlines() is more clear than read().splitlines() Even better - move the code to read the current rules into a method. current_rules = read_current_rules() Line 141: if os.path.isfile(osetupcons.FileLocations.OVIRT_IPTABLES_EXAMPLE): Line 142: with open( Line 143: osetupcons.FileLocations.OVIRT_IPTABLES_EXAMPLE, Line 144: 'r' Line 142: with open( Line 143: osetupcons.FileLocations.OVIRT_IPTABLES_EXAMPLE, Line 144: 'r' Line 145: ) as prev_setup_example: Line 146: prev_setup_rules = prev_setup_example.read().splitlines() This block should be handled like the previous read - best as a method: example_rules = read_example_config() Line 147: diff_prev_cur = difflib.unified_diff( Line 148: current_rules, Line 149: prev_setup_rules, Line 150: lineterm='', Line 148: current_rules, Line 149: prev_setup_rules, Line 150: lineterm='', Line 151: ) Line 152: diff_lines = '\n'.join(diff_prev_cur) This seems to the contents of a method named rule_differ(current_rules, example_rules) Line 153: if len(diff_lines) > 0: Line 154: current_modified_since_prev_setup = True Line 155: diff = difflib.unified_diff( Line 156: current_rules, Line 157: self._get_rules().splitlines(), Line 158: lineterm='', Line 159: fromfile=_('current'), Line 160: tofile=_('proposed'), Line 161: ) This should go into a method named create_diff() Can you show an example of this diff in the commit message? This looks pretty cool to me, but I wonder if diff output will be clear to the administrator running this tool. Line 162: diff_lines = '\n'.join(diff) Line 163: if len(diff_lines) > 0: Line 164: if current_modified_since_prev_setup: Line 165: self.logger.warning( Line 186: ), Line 187: prompt=True, Line 188: true=_('Yes'), Line 189: false=_('No'), Line 190: default=False, This block should go into some method like create_dialog() And it seems this code is trying to do: if self.environment["changes_review"] is None: self.environment["changes_review"] = create_dialog(...) This is not a good use of dictionary. If there are no review yet, it will be much nicer if there will be no key in the dictionary. Then this code can be: if "changes_review" not in self.enviroment: self.environment["changes_review"] = create_dialog(...) Or nicer: self.environment.setdefault("changes_review", create_dialog(...) It is very hard to understand the logic with so much code and in this indentation style. Line 191: ) Line 192: else: Line 193: interactive = False Line 194: Line 189: false=_('No'), Line 190: default=False, Line 191: ) Line 192: else: Line 193: interactive = False This else is so far away from the original if that there is no way to understand its meaning. Line 194: Line 195: if self.environment[ Line 196: osetupcons.ConfigEnv.FIREWALL_CHANGES_REVIEW Line 197: ] and interactive: Line 209: ), Line 210: prompt=True, Line 211: true=_('Yes'), Line 212: false=_('No'), Line 213: default=True, Should go into another dialog method. Line 214: ) Line 215: if not confirmed: Line 216: raise RuntimeError( Line 217: _( -- To view, visit http://gerrit.ovirt.org/34011 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8f16a90a074052226882d76fc80918ed276d379c Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.5 Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
