Alon Bar-Lev has posted comments on this change. Change subject: packaging: setup: cleanup nfs exports usage ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/24470/1//COMMIT_MSG Commit Message: Line 10: exports, move the entry to our own file. Line 11: Line 12: 2. if ACL specified enforce a new entry. Line 13: Line 14: 3. do not touch anything on upgrade unless (2). > Your current code does move the line even if not (2). yes it is, checked. Line 15: Line 16: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1058018 Line 17: Change-Id: Ie3df61b1b52aead5ddf457da14f26324dbc2a56c 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): > The code of these two functions is mostly duplicate. You could have used re I prefer duplication than complex 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): > Why all of this in validation and not misc? In a previous change you told m because I think we can have benefit in not finding what we expect and halt at this point. 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 > You said you want to check also ISO_DOMAIN_EXISTS what is the difference between that and ISO_DOMAIN_NFS_MOUNT_POINT is there is a cause when they will have different values? 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: ], > Did you verify all relevant scenarios? engine-setup; engine-setup; engine-c I think so, if you can find a sequence it does not work, I will be happy to attend. 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 > You know that already after setting _generate above. but I do not know the line. 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
