Alon Bar-Lev has posted comments on this change. Change subject: packaging: setup: cleanup nfs exports usage ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/24470/1/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/system/exportfs.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/system/exportfs.py: Line 50: $ Line 51: """, Line 52: ) Line 53: Line 54: def _getContentRemovePath(self, conf, path): > Not sure what's complex about it more than these two. have you seen it? it get 5 parameters, returns 3-4, has conditionals within. these functions are trivial, each does specific task, much easier to read and modify without breaking the entire code. Line 55: old_line = None Line 56: new_content = [] Line 57: if os.path.exists(conf): Line 58: with open(conf, 'r') as f: Line 115: @plugin.event( Line 116: stage=plugin.Stages.STAGE_VALIDATION, Line 117: condition=lambda self: self._enabled, Line 118: ) Line 119: def _validation(self): > But you never halt, do you? The only condition I can think of to halt is if for now I do not, if we even need to we do it here. Line 120: if os.path.exists(osetupcons.FileLocations.OVIRT_NFS_EXPORT_FILE): Line 121: self._source = self._destination = ( Line 122: osetupcons.FileLocations.OVIRT_NFS_EXPORT_FILE Line 123: ) Line 179: name=self._source, Line 180: content=content, Line 181: modifiedList=self.environment[ Line 182: otopicons.CoreEnv.MODIFIED_FILES Line 183: ], > > engine-cleanup deletes the file in exports.d although it contained manual >> engine-cleanup deletes the file in exports.d although it contained manual >> changes. > this is something we should avoid. If we move the line is at if it is our own. I do not wish to make it more complex. Line 184: ) Line 185: ) Line 186: Line 187: if not self._generate: Line 185: ) Line 186: Line 187: if not self._generate: Line 188: self._generate = True Line 189: new_line = old_line > Of course you know, unless I am missing something. The entire logic is here, no need to distribute it. Line 190: Line 191: self.logger.debug('generate=%s, line=%s', self._generate, new_line) Line 192: Line 193: if self._generate: -- To view, visit http://gerrit.ovirt.org/24470 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie3df61b1b52aead5ddf457da14f26324dbc2a56c Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
