[Tarantool-patches] [PATCH v4 3/3] Testing changes after 3rd review on S3
Igor Munkin
imun at tarantool.org
Mon Jan 13 19:54:46 MSK 2020
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
More information about the Tarantool-patches
mailing list