Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: cleanup nfs exports usage ......................................................................
Patch Set 1: (5 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): > I prefer duplication than complex code. Not sure what's complex about it more than these two. 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): > because I think we can have benefit in not finding what we expect and halt But you never halt, do you? The only condition I can think of to halt is if we find the path in both files, but you do not check this. 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 142: osetupcons.ConfigEnv.ISO_DOMAIN_NFS_MOUNT_POINT Line 143: ] is not None and Line 144: self.environment[ Line 145: osetupcons.ConfigEnv.ISO_DOMAIN_NFS_ACL Line 146: ] is not None > what is the difference between that and ISO_DOMAIN_NFS_MOUNT_POINT is there Currently the main difference is that ISO_DOMAIN_NFS_MOUNT_POINT is saved in the answer file, so is likely to be set even if there is no ISO domain. On a second check, you do implicitly check it - you do not save the ACL in the answer file, do check it, and it's set only if 'not ISO_DOMAIN_EXISTS'. I'd personally prefer to have it explicit but the functionality is ok. Line 147: ) Line 148: Line 149: self.logger.debug('move=%s, generate=%s', self._move, self._generate) Line 150: Line 179: name=self._source, Line 180: content=content, Line 181: modifiedList=self.environment[ Line 182: otopicons.CoreEnv.MODIFIED_FILES Line 183: ], > I think so, if you can find a sequence it does not work, I will be happy to Did some tests. The one that behaves differently from what I want: engine-setup with exports manually changing the acl mkdir exports.d engine-setup (moves the line) engine-cleanup engine-cleanup deletes the file in exports.d although it contained manual changes. And, BTW, you did not add comments ("Do not edit") to the file, but I do not think we'll add to to exports anyway, only to exports.d. 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 > but I do not know the line. Of course you know, unless I am missing something. 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
