Ofer Schreiber has posted comments on this change.
Change subject: packaging: Remote DB support - DO NOT MERGE!!!!.
......................................................................
Patch Set 10: Fails
(9 inline comments)
Multiple issues inline.
including:
1. Can't change password for remote DB auth during setup
2. Usage and prompt is unclear
and some more.
please make sure you handle previous comments as well.
....................................................
File packaging/fedora/setup/engine-setup.py
Line 290: { "CMD_OPTION" :"db-pass",
In this case, if there's an error while connecting to the DB, the user won't be
requested to enter the password again...
....................................................
File packaging/fedora/setup/engine-upgrade.py
Line 425: if DB_NAME_TEMP != "":
Any reason not to put this variable as a class one?
e.g = self.upgradeDbName or something like that?
Line 459: #cmd = "%s -s %s -p %s -d %s -u
%s"%(basedefs.FILE_DB_UPGRADE_SCRIPT, SERVER_NAME, SERVER_PORT,
basedefs.DB_NAME, SERVER_ADMIN)
you can remove this comments
Line 481: if utils.localHost(SERVER_NAME):
as I said before:
This logic should not be inside a function named "restartPostgresql"
Line 676: if type(e) != EnvironmentError:
This is not the right way to do so.
Why do you even need to do this check?
....................................................
File packaging/fedora/setup/engine_validators.py
Line 130: def validateRemoteHost(param, options=[]):
I'm not sure we want this kind of validation.
ping is not required.
the only thing we want is a valid DB connection.
Line 169: backupFile = "%s.%s" % (basedefs.DB_PASS_FILE,
utils.getCurrentDateTime())
any reason not to use env variable?
....................................................
File packaging/fedora/setup/output_messages.py
Line 125: INFO_CONF_PARAMS_USE_DB_HOST_USAGE="Please enter the host IP or host
name where DB is running. Use 'localhost' for default local installation"
This is not a valid usage
Line 126: INFO_CONF_PARAMS_USE_DB_HOST_PROMPT="Please enter the host IP or host
name where DB is running. Use 'localhost' for default local installation"
No need to use "Please".
--
To view, visit http://gerrit.ovirt.org/2245
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab66d6f8ffe33f9674e79753df7541c212012190
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Anonymous Coward #1000140
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Eyal Edri <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
