From: Igor Munkin <imun@tarantool.org> To: "Alexander V. Tikhonov" <avtikhon@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4 3/3] Testing changes after 3rd review on S3 Date: Mon, 13 Jan 2020 19:54:46 +0300 [thread overview] Message-ID: <20200113165446.GJ31304@tarantool.org> (raw) In-Reply-To: <2c726e7f73906789f4d863d7bd3f3d194a24a610.1578916717.git.avtikhon@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
next prev parent reply other threads:[~2020-01-13 16:57 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-13 12:04 [Tarantool-patches] [PATCH v4 0/3] gitlab-ci: implement packing into MCS S3 v.4 Alexander V. Tikhonov 2020-01-13 12:04 ` [Tarantool-patches] [PATCH v4 1/3] gitlab-ci: implement packing into MCS S3 Alexander V. Tikhonov 2020-01-13 12:04 ` [Tarantool-patches] [PATCH v4 2/3] Testing changes after 2nd review on S3 Alexander V. Tikhonov 2020-01-13 12:04 ` [Tarantool-patches] [PATCH v4 3/3] Testing changes after 3rd " Alexander V. Tikhonov 2020-01-13 16:54 ` Igor Munkin [this message] 2020-01-13 17:02 ` [Tarantool-patches] [PATCH v4 0/3] gitlab-ci: implement packing into MCS S3 v.4 Igor Munkin 2020-01-14 5:00 ` Alexander Tikhonov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200113165446.GJ31304@tarantool.org \ --to=imun@tarantool.org \ --cc=avtikhon@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4 3/3] Testing changes after 3rd review on S3' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox