Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: iso domain nfs export fixes ......................................................................
Patch Set 4: (4 comments) http://gerrit.ovirt.org/#/c/23975/4/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/system/exportfs.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/system/exportfs.py: Line 66: Line 67: def _nfs_export_path( Line 68: self, Line 69: conf, Line 70: path, > I still unsure why you insist of adding path/acl etc... you didn't want to "parse" these lines. Do you now? If we do, I'd not do this with split but with the regexp of 23736. Line 71: acl=None, Line 72: old_line=None, # Used if acl is None Line 73: overwrite=False Line 74: ): Line 68: self, Line 69: conf, Line 70: path, Line 71: acl=None, Line 72: old_line=None, # Used if acl is None > if you had to comment, probably something wrong. Yes, the wrong thing is that you objected to parsing... Line 73: overwrite=False Line 74: ): Line 75: uninstall_files = [] Line 76: exports_uninstall_group = self.environment[ Line 90: path=path, Line 91: acl=acl, Line 92: ) Line 93: ) Line 94: if overwrite: > why overwrite? if we find our path, we replace with ours... it can be same Again, in the case of exports.d/file you didn't want to check, parse, etc. - just overwrite. If we parse, we might as well do exactly the same as /etc/exports - edit in-line, no comment, addchanges etc. Will definitely simplify the code, at least until many years from now we'll not need to support /etc/exports (if at all). Line 95: # overwrite unconditionally with a suitable comment Line 96: content = [ Line 97: '# This file is automatically generated by engine-setup.', Line 98: '# Please do not edit manually.', Line 105: if index is not None: Line 106: changes['removed'] = content[index] Line 107: content[index] = new_line Line 108: else: Line 109: content.append(new_line) > I do not understand, we do not want to restore the line if moved/overwritte We do want to restore changes to /etc/exports. Perhaps I do not understand. Line 110: exports_uninstall_group.addChanges( Line 111: group='exportfs', Line 112: filename=conf, Line 113: changes=[changes], -- To view, visit http://gerrit.ovirt.org/23975 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia12c50b30c5df9f0da446ddbbf54c7ca73b7dd3d Gerrit-PatchSet: 4 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: 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
