Idan Shaby has posted comments on this change. Change subject: packaging: setup: SANWipeAfterDelete configuration ......................................................................
Patch Set 3: (9 comments) http://gerrit.ovirt.org/#/c/37397/3/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/config/storage.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/config/storage.py: Line 26: from otopi import util Line 27: from otopi import plugin Line 28: Line 29: Line 30: from ovirt_engine_setup.constants import ConfigEnv as configEnv > To be consistent with other plugins: Done Line 31: from ovirt_engine_setup.engine import constants as oenginecons Line 32: from ovirt_engine_setup.engine_common \ Line 33: import constants as oengcommcons Line 34: from ovirt_engine_setup import dialog Line 46: def _init(self): Line 47: self.environment.setdefault( Line 48: SAN_WIPE_AFTER_DELETE, Line 49: None Line 50: ) > Does it fit in one line? Done Line 51: Line 52: @plugin.event( Line 53: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 54: before=( Line 57: after=( Line 58: oengcommcons.Stages.DIALOG_TITLES_S_STORAGE, Line 59: ), Line 60: condition=lambda self: Line 61: self._newDatabaseCondition(), > This does not make sense - condition requires a function returning boolean. After talking to Nir, the current code seems to be the best way to do this, so it will remain the same. I fixed it to be in one line. Line 62: ) Line 63: def _configureSANWipeAfterDelete(self): Line 64: if self.environment[SAN_WIPE_AFTER_DELETE] is None: Line 65: # Value for SAN_WIPE_AFTER_DELETE is not forced. Line 61: self._newDatabaseCondition(), Line 62: ) Line 63: def _configureSANWipeAfterDelete(self): Line 64: if self.environment[SAN_WIPE_AFTER_DELETE] is None: Line 65: # Value for SAN_WIPE_AFTER_DELETE is not forced. > Better use this form: I think that the normal flow is more readable (and it fits to the 80 characters constraint). Line 66: sanWipeAfterDelete = dialog.queryBoolean( Line 67: dialog=self.dialog, Line 68: name='OVESETUP_CONFIG_SAN_WIPE_AFTER_DELETE', Line 69: note=_( Line 74: default=False, Line 75: ) Line 76: self.environment[ Line 77: SAN_WIPE_AFTER_DELETE Line 78: ] = sanWipeAfterDelete > Does it fit on one line? Done Line 79: Line 80: @plugin.event( Line 81: stage=plugin.Stages.STAGE_MISC, Line 82: after=( Line 82: after=( Line 83: oengcommcons.Stages.DB_CONNECTION_AVAILABLE, Line 84: ), Line 85: condition=lambda self: Line 86: self._newDatabaseCondition(), > Does it fit on the same line? Done Line 87: ) Line 88: def _updateSANWipeAfterDelete(self): Line 89: option = vdcoption.VdcOption( Line 90: statement=self.environment[ Line 88: def _updateSANWipeAfterDelete(self): Line 89: option = vdcoption.VdcOption( Line 90: statement=self.environment[ Line 91: oenginecons.EngineDBEnv.STATEMENT Line 92: ] > Does this fit on one line? Done Line 93: ) Line 94: options=( Line 95: { Line 96: 'name': 'SANWipeAfterDelete', Line 98: }, Line 99: ) Line 100: option.updateVdcOptions(options=options,) Line 101: Line 102: def _newDatabaseCondition(self): > How about _settingUpNewDatabase() ? What's wrong with the informative name _newDatabaseCondition? I think that _settingUpNewDatabase is confusing. Line 103: # Returns a condition that validates Line 104: # we are running the setup on a new database. Line 105: return self.environment[oenginecons.EngineDBEnv.NEW_DATABASE] Line 106: Line 102: def _newDatabaseCondition(self): Line 103: # Returns a condition that validates Line 104: # we are running the setup on a new database. Line 105: return self.environment[oenginecons.EngineDBEnv.NEW_DATABASE] Line 106: > 2 new lines here please Done -- 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: 3 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
