Re: [Tarantool-patches] [PATCH v4 3/3] Testing changes after 3rd review on S3

[Igor Munkin <imun@tarantool.org>]

Sasha,

Sorry, but I have to mention a couple remarks from my last review one
more time. Please consider them below.

On 13.01.20, Alexander V. Tikhonov wrote:
> ---
>  tools/add_pack_s3_repo.sh | 77 +++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 40 deletions(-)
> 

I wrote to the following in my previous review:
> General comments:
> * Please choose a single code style for function definition. Currently
>   you mix two following approaches:
> |  usage()
> |  {
>   and
> |  function pack_rpm {
As mentioned right above this note also related to pack_{deb,rpm}
functions, but both of them still violates the braces codestyle
considering the one you've chosen.

> diff --git a/tools/add_pack_s3_repo.sh b/tools/add_pack_s3_repo.sh
> index 280a07ab0..148ffd0d9 100755
> --- a/tools/add_pack_s3_repo.sh
> +++ b/tools/add_pack_s3_repo.sh
> @@ -11,13 +11,13 @@ product=tarantool
>  # the path with binaries either repository
>  repo=.
>  
> -get_os_dists()
> +function get_os_dists()
>  {
>      os=$1
>      alldists=
>  
>      if [ "$os" == "ubuntu" ]; then
> -        alldists='trusty xenial cosmic disco bionic eoan'
> +        alldists='trusty xenial bionic cosmic disco eoan'
>      elif [ "$os" == "debian" ]; then
>          alldists='jessie stretch buster'
>      elif [ "$os" == "el" ]; then
> @@ -29,13 +29,11 @@ get_os_dists()
>      echo "$alldists"
>  }
>  
> -prepare_ws()
> +function prepare_ws()
>  {
>      # temporary lock the publication to the repository
> -    ws=${ws_prefix}_${branch}_${os}
> -    if [ "$os" != "ubuntu" -a "$os" != "debian" ]; then
> -        ws=${ws}_${option_dist}
> -    fi
> +    ws_suffix=$1
> +    ws=${ws_prefix}_${branch}_${ws_suffix}
>      ws_lockfile=${ws}.lock
>      if [ -f $ws_lockfile ]; then
>          old_proc=$(cat $ws_lockfile)
> @@ -51,7 +49,7 @@ prepare_ws()
>      $mk_dir $ws
>  }
>  
> -usage()
> +function usage()
>  {
>      cat <<EOF
>  Usage for store package binaries from the given path:
> @@ -157,33 +155,34 @@ proddir=$(echo $product | head -c 1)
>  
>  # AWS defines
>  aws='aws --endpoint-url https://hb.bizmrg.com s3'
> -s3="s3://tarantool_repo/$branch/$os"
> -aws_cp_to_s3="$aws cp --acl public-read"
> -aws_sync_to_s3="$aws sync --acl public-read"
> +bucket_path="s3://tarantool_repo/$branch/$os"
> +aws_cp_to_s3_public="$aws cp --acl public-read"
> +aws_sync_to_s3_public="$aws sync --acl public-read"

I wrote to the following in my previous review:
>> Nice change, but other commands also interact with S3. I propose to
>> change the naming to the following one:
>> | <aws_<action>_public
To make it clearer a bit now I intended to drop _to_s3 suffix and left
something similar to the following:
| aws_cp_public="$aws cp --acl public-read"
| aws_sync_public="$aws sync --acl public-read"

>  
> -update_packfile()
> +function update_deb_packfile()
>  {
>      packfile=$1
>      packtype=$2
> +    update_dist=$3
>  
>      locpackfile=$(echo $packfile | sed "s#^$ws/##g")
>      # register DEB/DSC pack file to Packages/Sources file
> -    reprepro -Vb . include$packtype $loop_dist $packfile
> +    reprepro -Vb . include$packtype $update_dist $packfile
>      # reprepro copied DEB/DSC file to component which is not needed
>      $rm_dir $debdir/$component
>      # to have all sources avoid reprepro sMakinget DEB/DSC file to its own registry
>      $rm_dir db
>  }
>  
> -update_metadata()
> +function update_deb_metadata()
>  {
>      packpath=$1
>      packtype=$2
>  
>      if [ ! -f $packpath.saved ] ; then
>          # get the latest Sources file from S3
> -        $aws ls "$s3/$packpath" 2>/dev/null && \
> -            $aws cp "$s3/$packpath" $packpath.saved || \
> +        $aws ls "$bucket_path/$packpath" 2>/dev/null && \
> +            $aws cp "$bucket_path/$packpath" $packpath.saved || \
>              touch $packpath.saved
>      fi
>  
> @@ -236,7 +235,7 @@ function pack_deb {
>      fi
>  
>      # prepare the workspace
> -    prepare_ws
> +    prepare_ws ${os}
>  
>      # script works in one of 2 modes:
>      # - path with binaries packages for a single distribution
> @@ -257,8 +256,7 @@ function pack_deb {
>          exit 1
>      fi
>  
> -    swd=$PWD
> -    cd $ws
> +    pusha $ws
>  
>      # create the configuration file for 'reprepro' tool
>      confpath=$ws/conf
> @@ -292,17 +290,17 @@ EOF
>          for deb in $ws/$debdir/$loop_dist/$component/*/*/*.deb ; do
>              [ -f $deb ] || continue
>              # regenerate DEB pack
> -            update_packfile $deb deb
> +            update_deb_packfile $deb deb $loop_dist
>              echo "Regenerated DEB file: $locpackfile"
>              for packages in dists/$loop_dist/$component/binary-*/Packages ; do
>                  # copy Packages file to avoid of removing by the new DEB version
>                  # update metadata 'Packages' files
> -                if ! update_metadata $packages deb ; then continue ; fi
> +                ! update_deb_metadata $packages deb || continue
>                  updated_deb=1
>              done
>              # save the registered DEB file to S3
>              if [ "$updated_deb" == 1 ]; then
> -                $aws_cp_to_s3 $deb $s3/$locpackfile
> +                $aws_cp_to_s3_public $deb $bucket_path/$locpackfile
>              fi
>          done
>  
> @@ -310,19 +308,19 @@ EOF
>          for dsc in $ws/$debdir/$loop_dist/$component/*/*/*.dsc ; do
>              [ -f $dsc ] || continue
>              # regenerate DSC pack
> -            update_packfile $dsc dsc
> +            update_deb_packfile $dsc dsc $loop_dist
>              echo "Regenerated DSC file: $locpackfile"
>              # copy Sources file to avoid of removing by the new DSC version
>              # update metadata 'Sources' file
> -            if ! update_metadata dists/$loop_dist/$component/source/Sources dsc \
> -                ; then continue ; fi
> +            ! update_deb_metadata dists/$loop_dist/$component/source/Sources dsc \
> +                || continue
>              updated_dsc=1
>              # save the registered DSC file to S3
> -            $aws_cp_to_s3 $dsc $s3/$locpackfile
> +            $aws_cp_to_s3_public $dsc $bucket_path/$locpackfile
>              tarxz=$(echo $locpackfile | sed 's#\.dsc$#.debian.tar.xz#g')
> -            $aws_cp_to_s3 $ws/$tarxz "$s3/$tarxz"
> +            $aws_cp_to_s3_public $ws/$tarxz "$bucket_path/$tarxz"
>              orig=$(echo $locpackfile | sed 's#-1\.dsc$#.orig.tar.xz#g')
> -            $aws_cp_to_s3 $ws/$orig "$s3/$orig"
> +            $aws_cp_to_s3_public $ws/$orig "$bucket_path/$orig"
>          done
>  
>          # check if any DEB/DSC files were newly registered
> @@ -390,13 +388,13 @@ EOF
>          popd
>  
>          # 4. sync the latest distribution path changes to S3
> -        $aws_sync_to_s3 dists/$loop_dist "$s3/dists/$loop_dist"
> +        $aws_sync_to_s3_public dists/$loop_dist "$bucket_path/dists/$loop_dist"
>      done
>  
>      # unlock the publishing
>      $rm_file $ws_lockfile
>  
> -    cd $swd
> +    popd
>  }
>  
>  # The 'pack_rpm' function especialy created for RPM packages. It works
> @@ -416,13 +414,12 @@ function pack_rpm {
>      fi
>  
>      # prepare the workspace
> -    prepare_ws
> +    prepare_ws ${os}_${option_dist}
>  
>      # copy the needed package binaries to the workspace
>      cp $repo/*.rpm $ws/.
>  
> -    swd=$PWD
> -    cd $ws
> +    pushd $ws
>  
>      # set the paths
>      if [ "$os" == "el" ]; then
> @@ -437,13 +434,13 @@ function pack_rpm {
>      # prepare local repository with packages
>      $mk_dir $packpath
>      mv *.rpm $packpath/.
> -    cd $ws/$repopath
> +    cd $repopath
>  
>      # copy the current metadata files from S3
>      mkdir repodata.base
> -    for file in $($aws ls $s3/$repopath/repodata/ | awk '{print $NF}') ; do
> -        $aws ls $s3/$repopath/repodata/$file || continue
> -        $aws cp $s3/$repopath/repodata/$file repodata.base/$file
> +    for file in $($aws ls $bucket_path/$repopath/repodata/ | awk '{print $NF}') ; do
> +        $aws ls $bucket_path/$repopath/repodata/$file || continue
> +        $aws cp $bucket_path/$repopath/repodata/$file repodata.base/$file
>      done
>  
>      # create the new repository metadata files
> @@ -507,16 +504,16 @@ EOF
>  
>      # copy the packages to S3
>      for file in $rpmpath/*.rpm ; do
> -        $aws_cp_to_s3 $file "$s3/$repopath/$file"
> +        $aws_cp_to_s3_public $file "$bucket_path/$repopath/$file"
>      done
>  
>      # update the metadata at the S3
> -    $aws_sync_to_s3 repodata "$s3/$repopath/repodata"
> +    $aws_sync_to_s3_public repodata "$bucket_path/$repopath/repodata"
>  
>      # unlock the publishing
>      $rm_file $ws_lockfile
>  
> -    cd $swd
> +    popd
>  }
>  
>  if [ "$os" == "ubuntu" -o "$os" == "debian" ]; then
> -- 
> 2.17.1
> 

-- 
Best regards,
IM