Idan Shaby has posted comments on this change. Change subject: packaging: setup: SANWipeAfterDelete configuration ......................................................................
Patch Set 1: (10 comments) http://gerrit.ovirt.org/#/c/37397/1/packaging/setup/ovirt_engine_setup/constants.py File packaging/setup/ovirt_engine_setup/constants.py: Line 358: def STORAGE_IS_LOCAL(self): Line 359: return 'OVESETUP_CONFIG/storageIsLocal' Line 360: Line 361: @osetupattrs( Line 362: answerfile=True, > Not sure that we need this, since it is needed only when creating a new dat Done Line 363: summary=True, Line 364: description=_('SAN wipe after delete'), Line 365: postinstallfile=True, Line 366: ) Line 361: @osetupattrs( Line 362: answerfile=True, Line 363: summary=True, Line 364: description=_('SAN wipe after delete'), Line 365: postinstallfile=True, > Do we need this? Yes, if you delete this - you won't see SANWipeAfterDelete in the "CONFIGURATION PREVIEW" section the next time you run the setup. Line 366: ) Line 367: def SAN_WIPE_AFTER_DELETE(self): Line 368: return 'OVESETUP_CONFIG/sanWipeAfterDelete' Line 369: http://gerrit.ovirt.org/#/c/37397/1/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine-common/dialog/titles.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine-common/dialog/titles.py: Line 206: oengcommcons.Stages.DIALOG_TITLES_S_STORAGE, Line 207: ), Line 208: ) Line 209: def _title_e_storage(self): Line 210: pass > Do we need this? Other plugins are using this, but it looks pointless. I guess we do. Simone, maybe you can tell Nir why we need it? Line 211: Line 212: @plugin.event( Line 213: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 214: name=oengcommcons.Stages.DIALOG_TITLES_S_PKI, http://gerrit.ovirt.org/#/c/37397/1/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/config/storage.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/config/storage.py: Line 41: Line 42: @plugin.event( Line 43: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 44: before=( Line 45: oengcommcons.Stages.DIALOG_TITLES_E_STORAGE, > Why is this a tuple? Can we define one value instead? Maybe we can, but I think that it is done this way so you can always add it some more arguments. Line 46: ), Line 47: after=( Line 48: oengcommcons.Stages.DIALOG_TITLES_S_STORAGE, Line 49: ), Line 44: before=( Line 45: oengcommcons.Stages.DIALOG_TITLES_E_STORAGE, Line 46: ), Line 47: after=( Line 48: oengcommcons.Stages.DIALOG_TITLES_S_STORAGE, > Same Please see my answer above. Line 49: ), Line 50: condition=lambda self: Line 51: self.environment[oenginecons.EngineDBEnv.NEW_DATABASE], Line 52: ) Line 46: ), Line 47: after=( Line 48: oengcommcons.Stages.DIALOG_TITLES_S_STORAGE, Line 49: ), Line 50: condition=lambda self: > Please add a comment explaining this setting. Done Line 51: self.environment[oenginecons.EngineDBEnv.NEW_DATABASE], Line 52: ) Line 53: def _configureSANWipeAfterDelete(self): Line 54: sanWipeAfterDelete = dialog.queryBoolean( Line 50: condition=lambda self: Line 51: self.environment[oenginecons.EngineDBEnv.NEW_DATABASE], Line 52: ) Line 53: def _configureSANWipeAfterDelete(self): Line 54: sanWipeAfterDelete = dialog.queryBoolean( > In this context, there is one query returning one value, so there is no nee I disagree. Why using such an abstract and uninformative name? And what would the next person who edits this code do when he will need to add another call to queryBoolean? He will rename 'value' to sanWipeAfterDelete, or call the new one 'value2', which is even worse. Thus, I think that it should remain sanWipeAfterDelete. Line 55: dialog=self.dialog, Line 56: name='OVESETUP_CONFIG_SAN_WIPE_AFTER_DELETE', Line 57: note=_( Line 58: 'SAN wipe after delete ' Line 62: default=False, Line 63: ) Line 64: self.environment[ Line 65: osetupcons.ConfigEnv.SAN_WIPE_AFTER_DELETE Line 66: ] = sanWipeAfterDelete > This is not readable way to format code. Done Line 67: Line 68: @plugin.event( Line 69: stage=plugin.Stages.STAGE_MISC, Line 70: after=( Line 70: after=( Line 71: oengcommcons.Stages.DB_CONNECTION_AVAILABLE, Line 72: ), Line 73: condition=lambda self: Line 74: self.environment[oenginecons.EngineDBEnv.NEW_DATABASE], > This repeats the same condition as in the other method - consider adding a Done Line 75: ) Line 76: def _updateSANWipeAfterDelete(self): Line 77: vdcoption.VdcOption( Line 78: statement=self.environment[ Line 86: osetupcons.ConfigEnv.SAN_WIPE_AFTER_DELETE Line 87: ], Line 88: }, Line 89: ), Line 90: ) > This is bad style that make the code unreadable. There is no reason to use Done Line 91: Line 92: -- To view, visit http://gerrit.ovirt.org/37397 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I911f87ad34eafc83eb2c7aa346fc4278ada28b5b Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Candace Sheremeta <csher...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
