[Ovirt-devel] [PATCH ovirt-appliance] make gettree a bit smarted

[Jim Meyering]

Alan Pevec <apevec at redhat.com> wrote:
> handle both F9 and F10 trees now by using .treeinfo data

Hi Alan,

Nice patch!

There was one fatal typo.
I've suggested a fix for that,
along with a couple other minor things.

There's a nit in subject: s/ted$/ter/

> ---
>  gettree.sh         |   69 ++++++++++++++++++++++++++++++++++++++-------------
>  ovirt-appliance.ks |   11 +-------
>  2 files changed, 52 insertions(+), 28 deletions(-)
>
> diff --git a/gettree.sh b/gettree.sh
> index 67f6cf9..2310f2d 100755
> --- a/gettree.sh
> +++ b/gettree.sh
> @@ -5,13 +5,32 @@
>  #     e.g. http://download.fedoraproject.org/pub/fedora/linux/releases/9/Fedora/x86_64/os
>  #   download minimal Fedora tree: .treeinfo stage2 initrd and kernel
>
> +# Requires: wget
> +# Requires: python-iniparse
> +# Requires: createrepo
> +
> +
> +# download $destination $URL [$alternative1 ...]
> +# attempt to download from a list of URLs until it succeeds
>  download() {
> -    local f=$1
> -    case "$f" in
> -        file://*) cp ${f#file://} . ;;
> -        *) wget --progress=dot:mega --continue $f ;;
> -    esac
> -    printf "."
> +    local destination=$1
> +    shift
> +    local files="$@"
> +    set +e
> +    rc=0
> +    for f in $files; do
> +        case "$f" in

quotes not needed in "case":

           case $f in

> +            file://*) cp ${f#file://} "$destination" ;;
> +            *) wget --directory-prefix="$destination" ---progress=dot:mega --continue "$f" ;;

Too many "-": s/---/--/

And that line is too long, so:

               *) wget --directory-prefix="$destination" \
                    --progress=dot:mega --continue "$f" ;;

> +        esac
> +        rc=$?
> +        if [ $rc == 0 ]; then
> +            printf "."
> +            break
> +        fi
> +    done
> +    set -e
> +    return $rc
>  }
>
>  if [[ $# < 2 ]]; then
> @@ -24,20 +43,34 @@ fi
>  url=$1
>  dest=$2
>
> -pushd $dest
>  printf "Downloading minimal Fedora install tree from $url"
>  set -e
> -download $url/.treeinfo
> +download "$dest" $url/.treeinfo
> +python -c '
> +from iniparse.ini import INIConfig
> +ini = INIConfig()
> +fp = open(".treeinfo")
> +ini.readfp(fp)
> +fp.close()
> +family = ini.general.family
> +version = ini.general.version
> +arch = ini.general.arch
> +kernel = ini['images-'+arch].kernel
> +initrd = ini['images-'+arch].initrd
> +stage2 = ini.stage2.mainimage
> +print "%s %s %s" % (family, version, arch, kernel, initrd, stage2)' | ( read os ver arch kernel initrd stage2
> +echo $os $ver $arch > .treeinfo.ova
>  mkdir -p Packages
> -cd Packages
> -download $url/Packages/basesystem-8.1-1.noarch.rpm
> -cd ..
> +download Packages $url/Packages/basesystem-8.1-1.noarch.rpm $url/Packages/basesystem-10.0-1.noarch.rpm

To be safe, better to quote things and split long lines for readability:

download Packages "$url/Packages/basesystem-8.1-1.noarch.rpm" \
                  "$url/Packages/basesystem-10.0-1.noarch.rpm"

>  createrepo .
> -mkdir -p images/pxeboot
> -cd images
> -download $url/images/stage2.img
> -cd pxeboot
> -download $url/images/pxeboot/initrd.img
> -download $url/images/pxeboot/vmlinuz
> +dir="$dest/$(dirname $stage2)"
> +mkdir -p "$dir"
> +download "$dir" $url/$stage2

Add quotes:
download "$dir" "$url/$stage2"

> +dir="$dest/$(dirname $kernel)"
> +mkdir -p "$dir"
> +download "$dir" $url/$kernel

And here             ^^

> +dir="$dest/$(dirname $initrd)"
> +mkdir -p "$dir"
> +download "$dir" $url/$initrd

and here
download "$dir" "$url/$initrd"

> +)
>  echo "done"
> -popd
> diff --git a/ovirt-appliance.ks b/ovirt-appliance.ks
> index 8fabdb7..e9c19be 100644
> --- a/ovirt-appliance.ks
> +++ b/ovirt-appliance.ks
> @@ -45,16 +45,7 @@ lokkit
>
>  %post --nochroot
>    set -e
> -  python -c '
> -from iniparse.ini import INIConfig
> -ini = INIConfig()
> -fp = open("tmp/tree/.treeinfo")
> -ini.readfp(fp)
> -fp.close()
> -family = ini.general.family
> -version = ini.general.version
> -arch = ini.general.arch
> -print "%s %s %s" % (family, version, arch)' | ( read os ver arch
> +  cat tmp/tree/.treeinfo.ova | ( read os ver arch
>        dest=$INSTALL_ROOT/var/www/cobbler/ks_mirror/$os-$ver-$arch
>        printf "Importing $os-$ver-$arch ..."
>        cp -a tmp/tree $dest

I see it's just context, but this should be

        cp -a tmp/tree "$dest"