[Ovirt-devel] [Patch] oVirt Autobuild

[Jim Meyering]

Mohammed Morsi <mmorsi at redhat.com> wrote:
>>From c1ebbe270bd02937e09a85518ea65470d7bb2ea5 Mon Sep 17 00:00:00 2001
> From: Mohammed Morsi <mmorsi at redhat.com>
> Date: Mon, 21 Jul 2008 21:18:24 -0400
> Subject: [PATCH] changes to get ovirt / autobuild working
...
> +# determine if we're on the wui appliance or not
> +hn=`hostname`
> +if [ ! "$hn" == "management.priv.ovirt.org" ]; then

Hi Mo,

Here are a few suggestions:

> +     # delete previous appliance, restart libvirt
> +     if [ -f /etc/libvirt/qemu/ovirt-appliance.xml ]; then
> +     	rm -f /etc/libvirt/qemu/ovirt-appliance.xml
> +     fi
> +     if [ -f /var/lib/libvirt/images/ovirt-appliance.img ]; then
> +        rm -f /var/lib/libvirt/images/ovirt-appliance.img
> +     fi

Just use rm -f.  No need to test for existence first:

    rm -f /etc/libvirt/qemu/ovirt-appliance.xml
    rm -f /var/lib/libvirt/images/ovirt-appliance.img

Actually, with the former name appearing once above and twice below,
it's long enough that it deserves to be factored out.

> +     /sbin/service libvirtd restart
> +
> +     # create appliance
> +    ./build-all.sh -ac
> +
> +     # wait until the install is complete
> +     status=`virsh domstate ovirt-appliance`
> +     while [ ! -f /etc/libvirt/qemu/ovirt-appliance.xml -o "$status" != "shut off" ]
> +     do
> +         sleep 10
> +         status=`virsh domstate ovirt-appliance`
> +     done

I find such loops more readable/maintainable without the repeated
`...` command and with positive tests:

    while : ; do
      status=`virsh domstate ovirt-appliance`
      test "$status" = 'shut off' && break
      test -f /etc/libvirt/qemu/ovirt-appliance.xml && break
      sleep 10
    done

...
> +     ssh root at 192.168.50.2 "sed -i -e \"s/ADMIN_EMAIL/$OAB_ADMIN_EMAIL/g\" /etc/auto-build.d/auto-build.conf &&
> +	                    sed -i -e \"s/ADMIN_NAME/$OAB_ADMIN_NAME/g\" /etc/auto-build.d/auto-build.conf &&
> +	                    sed -i -e \"s/GROUP_EMAIL/$OAB_GROUP_EMAIL/g\" /etc/auto-build.d/auto-build.conf &&
> +	                    sed -i -e \"s/GROUP_NAME/$OAB_GROUP_NAME/g\" /etc/auto-build.d/auto-build.conf &&
> +	                    sed -i -e \"s/HOSTNAME/$OAB_HOSTNAME/g\" /etc/auto-build.d/auto-build.conf"
> +     # run the wui auto-build
> +     echo 'Starting oVirt Wui Autobuild'
> +     ssh root at 192.168.50.2 "auto-build-make-root &&
> +			       auto-build --config /etc/auto-build.d/auto-build.conf"

We can eliminate some of those long lines and duplication.
This should be mostly equivalent:

    echo 'Starting oVirt Wui Autobuild'
    autobuild_conf=/etc/auto-build.d/auto-build.conf
    ssh root at 192.168.50.2 "sed -i
        -e \"s/ADMIN_EMAIL/$OAB_ADMIN_EMAIL/g\"
        -e \"s/ADMIN_NAME/$OAB_ADMIN_NAME/g\"
        -e \"s/GROUP_EMAIL/$OAB_GROUP_EMAIL/g\"
        -e \"s/GROUP_NAME/$OAB_GROUP_NAME/g\"
        -e \"s/HOSTNAME/$OAB_HOSTNAME/g\" $autobuild_conf &&
      auto-build-make-root &&
      auto-build --config $autobuild_conf"

> +     # mount the results in an httpd accessible location
> +     if [ ! -d /var/www/html/builder-wui/ ]; then
> +       mkdir /var/www/html/builder-wui/
> +     fi

No need for the existence test.  Use -p instead:

    mkdir -p /var/www/html/builder-wui

> +     sshfs -o allow_other 192.168.50.2:/var/lib/builder/public_html/ /var/www/html/builder-wui/
> +
> +else
> +     # enter wui dir and run tests
> +     cd wui/src/
> +     rake test
> +fi