Simone Tiraboschi has posted comments on this change. Change subject: packaging: setup: asking about retriyng VG creation with force option ......................................................................
Patch Set 7: (8 comments) http://gerrit.ovirt.org/#/c/37845/7/src/plugins/ovirt-hosted-engine-setup/storage/blockd.py File src/plugins/ovirt-hosted-engine-setup/storage/blockd.py: Line 271: prompt=True, Line 272: validValues=(_('Force'), _('Abort')), Line 273: caseSensitive=False, Line 274: default=_('Abort'), Line 275: ).lower() == _('Force').lower() > If the force option is specified externally, we apply this option without a Yes, we ask only if the if self.environment[ohostedcons.StorageEnv.FORCE_CREATEVG] is None It's initialized as the external supplied value or None elsewhere. Line 276: Line 277: def _create_vg(self, forceVG=False): Line 278: return self.cli.createVG( Line 279: self.environment[ohostedcons.StorageEnv.SD_UUID], Line 600: forceVG = False Line 601: if self.environment[ Line 602: ohostedcons.StorageEnv.FORCE_CREATEVG Line 603: ]: Line 604: forceVG = True > This is equivalent to : I prefer to initialize to the external supplied value or None otherwise and simply ask if None. Line 605: self.logger.info(_('Creating Volume Group')) Line 606: dom = self._create_vg(forceVG) Line 607: self.logger.debug(dom) Line 608: if dom['status']['code'] != 0: Line 611: 'Error creating Volume Group: {message}' Line 612: ).format( Line 613: message=dom['status']['message'] Line 614: ) Line 615: ) > This error is relevant only if forceVG was False, meaning that the vg is fr It's still an error and I think that is a good idea to show it to the user. self.logger.error it's a bit misleading cause it's not just for the log but it's also printed to the user on the CLI console. Line 616: if not forceVG: Line 617: # eventually retry forcing VG creation on dirty storage Line 618: self._customize_forcecreatevg() Line 619: if not self.environment[ Line 612: ).format( Line 613: message=dom['status']['message'] Line 614: ) Line 615: ) Line 616: if not forceVG: > First handle the error case - It is much eaiser to follow: Done Line 617: # eventually retry forcing VG creation on dirty storage Line 618: self._customize_forcecreatevg() Line 619: if not self.environment[ Line 620: ohostedcons.StorageEnv.FORCE_CREATEVG Line 618: self._customize_forcecreatevg() Line 619: if not self.environment[ Line 620: ohostedcons.StorageEnv.FORCE_CREATEVG Line 621: ]: Line 622: raise RuntimeError(dom['status']['message']) > Why do you need to _customize the force flag at this stage? Returning an user answer though the environment is useful cause all the environment values are clearly tracked in the debug logs and so it make easier for us to reconstruct the flow on errors. We need to ask again here cause, a you pointed out, we got a value from outside or we just asked before if and only if the LUN was reported as used and the vgUUID was empty. In every other case we still have to ask. I simply used the same support method cause the two dialog are really similar and the user has just been alerted about the error by self.logger.error Line 623: else: Line 624: forceVG = True Line 625: dom = self._create_vg(forceVG) Line 626: self.logger.debug(dom) Line 620: ohostedcons.StorageEnv.FORCE_CREATEVG Line 621: ]: Line 622: raise RuntimeError(dom['status']['message']) Line 623: else: Line 624: forceVG = True > Setting the temporary just to call the function is pointless. Done Line 625: dom = self._create_vg(forceVG) Line 626: self.logger.debug(dom) Line 627: if dom['status']['code'] != 0: Line 628: self.logger.error( Line 630: 'Error creating Volume Group: {message}' Line 631: ).format( Line 632: message=dom['status']['message'] Line 633: ) Line 634: ) > You duplicate here not only the [status][code] but also the logging code. self.logger.error it's also printed to the user on the CLI console. Line 635: raise RuntimeError(dom['status']['message']) Line 636: else: Line 637: raise RuntimeError(dom['status']['message']) Line 638: self.environment[ Line 633: ) Line 634: ) Line 635: raise RuntimeError(dom['status']['message']) Line 636: else: Line 637: raise RuntimeError(dom['status']['message']) > Last, you mix words_with_underscore (_create_vg), joinedwords (_custimize_f Done Line 638: self.environment[ Line 639: ohostedcons.StorageEnv.VG_UUID Line 640: ] = dom['uuid'] Line 641: -- To view, visit http://gerrit.ovirt.org/37845 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I496c34e6b9f0d84443a8d5bc68d77916be6cb504 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
