From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4D87C46970E for ; Mon, 13 Jan 2020 19:57:01 +0300 (MSK) Date: Mon, 13 Jan 2020 19:54:46 +0300 From: Igor Munkin Message-ID: <20200113165446.GJ31304@tarantool.org> References: <2c726e7f73906789f4d863d7bd3f3d194a24a610.1578916717.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2c726e7f73906789f4d863d7bd3f3d194a24a610.1578916717.git.avtikhon@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 3/3] Testing changes after 3rd review on S3 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: tarantool-patches@dev.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 < 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: >> | _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