Alon Bar-Lev has posted comments on this change.

Change subject: packaging: Allow restore to different db location
......................................................................


Patch Set 3:

(9 comments)

....................................................
File packaging/bin/engine-backup.sh
Line 151
Line 152
Line 153
Line 154
Line 155
folders are now files, no?


Line 29: /etc/httpd/conf.d/z-ovirt-engine-proxy.conf
Line 30: /etc/yum/pluginconf.d/versionlock.list
Line 31: /etc/firewalld/services/ovirt-https.xml
Line 32: /etc/firewalld/services/ovirt-http.xml
Line 33: /etc/firewalld/services/ovirt-postgres.xml"
please separate to different patch.
Line 34: MYPGPASS=""
Line 35: TEMP_FOLDER=""
Line 36: FILE=""
Line 37: DB_BACKUP_FILE_NAME="engine_backup.db"


Line 111:                               DB_PASSFILE="${v}"
Line 112:                               if ! [ -r "${DB_PASSFILE}" ]; then
Line 113:                                       die "Can not read password file 
${DB_PASSFILE}"
Line 114:                               fi
Line 115:                               read DB_PASSWORD < "${DB_PASSFILE}"
please support also plain password from cmdline
Line 116:                       ;;
Line 117:                       --db-name=*)
Line 118:                               DB_NAME="${v}"
Line 119:                       ;;


Line 119:                       ;;
Line 120:                       --db-secured=*)
Line 121:                               DB_SECURED="${v}"
Line 122:                               case "${DB_SECURED}" in
Line 123:                                       True|False) ;;
this can be boolean parameter --db-secured sets True while default is False
Line 124:                                       *) die "invalid 'sec' value 
'${DB_SECURED}' for --db-secured"
Line 125:                               esac
Line 126:                       ;;
Line 127:                       --db-sec-validation=*)


Line 123:                                       True|False) ;;
Line 124:                                       *) die "invalid 'sec' value 
'${DB_SECURED}' for --db-secured"
Line 125:                               esac
Line 126:                       ;;
Line 127:                       --db-sec-validation=*)
same... boolean
Line 128:                               DB_SEC_VALIDATION="${v}"
Line 129:                               case "${DB_SEC_VALIDATION}" in
Line 130:                                       True|False) ;;
Line 131:                                       *) die "invalid 'valid' value 
'${DB_SEC_VALIDATION}' for --db-sec-validation"


Line 250:       verifyConnection
Line 251:       log "Restoring database backup at 
${TEMP_FOLDER}/db/${DB_BACKUP_FILE_NAME}"
Line 252:       restoreDB "${TEMP_FOLDER}/db/${DB_BACKUP_FILE_NAME}"
Line 253:       output "Starting services:"
Line 254:       service ovirt-engine start
I am not sure we need to start automatically, maybe there are manual task to do 
before.
Line 255:       service httpd restart
Line 256:       output "Note: you might need to manually fix firewalld conf and 
autostart of ovirt-engine service"
Line 257: }
Line 258: 


Line 296:               if [ -e "${backup}" ]; then
Line 297:                       cp -a "${backup}" "${dirname}" || logdie 
"Cannot copy '${backup}' to '${dirname}'"
Line 298:                       selinuxenabled && restorecon -R "${folder}"
Line 299:               else
Line 300:                       echo "Skipping not-existent ${folder}"
usually, the short side/non operation block of the condition - better to place 
it first.
Line 301:               fi
Line 302:       done || logdie "Cannot read ${folders}"
Line 303:       changeDBConf
Line 304: }


Line 314:       DB_USER
Line 315:       DB_PASSWORD
Line 316:       DB_NAME
Line 317:       DB_SECURED
Line 318:       DB_SEC_VALIDATION"
please indent this, to something like:

     local vars="
          var1
          var2
     "

this will be clear that vars are part of block and will enable adding more at 
future in clean diff
Line 319: 
Line 320:       echo "${vars}" | while read var; do
Line 321:               eval "value=\${$var}"
Line 322:               if [ -n "${value}" ]; then


Line 316:       DB_NAME
Line 317:       DB_SECURED
Line 318:       DB_SEC_VALIDATION"
Line 319: 
Line 320:       echo "${vars}" | while read var; do
I am unsure why not just create our own file from scratch if we were provided 
external credentials.
Line 321:               eval "value=\${$var}"
Line 322:               if [ -n "${value}" ]; then
Line 323:                       if [ "${enabled}" = "0" ]; then
Line 324:                               log "Backing up ${conf} to ${backup}"


-- 
To view, visit http://gerrit.ovirt.org/20258
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2c84111808c638a1c589cf3c14c271184a0a7c2
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yedidyah Bar David <d...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@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

Reply via email to