Alexander, thanks a lot for a deep review, I've answered the questions below. >Вторник, 18 февраля 2020, 20:08 +03:00 от Alexander Turenko : > >> Make S3 paths the same to Tarantool documented > >Please, add a prefix: gitlab-ci, infra or so. Before we use 'gitlab-ci'. > >'S3 paths' does not say anything about change you doing. Is it about >tarballs? Or packages? Or some other part of infrastructure? > >> Changed the Vanilla paths of the packages in S3 to >> the Tarantool documented on its web site, to give >> the ability to Tarantool users to continue use it >> w/o changes. > >Here you introduce the term 'the Vanilla paths' (which is used as >'Vanila' below), but it is neither commonly known nor explained. In fact >you meant base URL of main CentOS/Fedora repository. Since we providing >our own repository it obviously will have its own base URL. > >> >> Also moved the RPM sources packages to the standalone >> path. >> >> Follows up #3380 > >Maybe like so: > > | gitlab-ci: adjust base URL of RPM/Deb repositories > | > | Our S3 based repositories now reflect packagecloud.io repositories > | structure. > | > | It will allow us to migrate from packagecloud.io w/o much complicating > | redirection rules on a web server serving download.tarantool.org. > | > | Deploy source packages (*.src.rpm) into separate 'SRPM' repository > | like packagecloud.io does. > | > | > | > | Follows up #3380 > >See, I didn't introduced any new term, but described why and how the >behaviour of the script was changed. Thank for the complete commit message, I've used it completely and added comment about changed signing key. > >> --- >> >> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-3380-rpm-packages-paths-full-ci >> Issue: https://github.com/tarantool/tarantool/issues/3380 >> >> tools/update_repo.sh | 73 +++++++++++++++++++++++++++++++------------- >> 1 file changed, 51 insertions(+), 22 deletions(-) >> >> diff --git a/tools/update_repo.sh b/tools/update_repo.sh >> index 65a977187..ec6146514 100755 >> --- a/tools/update_repo.sh >> +++ b/tools/update_repo.sh >> @@ -405,42 +405,68 @@ EOF >> } >> >> # The 'pack_rpm' function especialy created for RPM packages. It works >> -# with RPM packing OS like Centos, Fedora. It is based on globaly known >> +# with RPM packing OS like CentOS, Fedora. It is based on globaly known > >If you want to fix typos, then fix also 'globaly'. Right, corrected. > > >> # tool 'createrepo' from: >> -# https://linux.die.net/man/8/createrepo >> +# https://linux.die.net/man/8/createrepo >> # This tool works with single distribution of the given OS. >> -# Result of the routine is the rpm package for YUM repository with >> -# file structure equal to the Centos/Fedora: >> -# http://mirror.centos.org/centos/7/os/x86_64/Packages/ >> -# http://mirrors.kernel.org/fedora/releases/30/Everything/x86_64/os/Packages/t/ >> +# >> +# Vanilla RPM packages structure for YUM repositories looks like: >> +# CentOS: >> +# Binaries: http://mirror.centos.org/centos/7/os/x86_64/Packages/ >> +# Sources: http://vault.centos.org/8.1.1911/BaseOS/Source/SPackages/ >> +# Fedora: >> +# Binaries: http://mirrors.kernel.org/fedora/releases/30/Everything/x86_64/os/Packages/t/ >> +# Sources: http://mirrors.kernel.org/fedora/releases/30/Everything/source/tree/Packages/t/ >> +# Decided to have back port capability at Tarantool links: >> +# CentOS: >> +# Tarantool: https://www.tarantool.io/en/download/os-installation/rhel-centos/ >> +# Changed Vanila -> MCS S3: >> +# centos/7/os/x86_64/Packages -> centos/7/x86_64/Packages >> +# 8.1.1911/BaseOS/Source/SPackages -> centos/8.1.1911/SRPMS/Packages >> +# Fedora: >> +# Tarantool: https://www.tarantool.io/en/download/os-installation/fedora/ >> +# Changed Vanila -> MCS S3: >> +# fedora/releases/30/Everything/x86_64/os/Packages -> fedora/30/x86_64/Packages >> +# fedora/releases/30/Everything/source/tree/Packages -> fedora/30/SRPMS/Packages > >If we agree to don't bother anybody to imagine what 'Vanilla/Vanila' is >(and what is meant under 'MCS S3', which is just file storage), then >let's just show base URLs. > >However it is okay to give links to intructions, which will allow to >verify that the URLs are right. Ok, I've removed all links and information about OS's structures and wrote links on the Tarantool instructions. > > >> function pack_rpm { >> - if ! ls $repo/*.rpm >/dev/null ; then >> - echo "ERROR: Current '$repo' path doesn't have RPM packages in path" >> - usage >> - exit 1 >> - fi >> + pack_subdir=$1 >> + pack_patterns=$2 >> + >> + for pack_pattern in $pack_patterns ; do >> + if ! ls $repo/$pack_pattern >/dev/null ; then >> + if [ "$pack_pattern" == "*.noarch.rpm" ]; then >> + pack_patterns=$(echo "$pack_patterns" | sed "s#$pack_pattern##g") >> + continue >> + fi >> + echo "ERROR: Current '$repo' path doesn't have $pack_pattern packages in path" >> + usage >> + exit 1 >> + fi >> + done > >This logic is flawed (say, it will not work with vshard). Okay, let's >give an error if there are no files, but it should not require files to >be present for a certain architecture. Ok, I see, I've fixed it for all the patterns and set checker that some packages found to be registered. > > >I would collect files using globs. Then check whether we found at least >one. Then use this list of files. A kind of this: > > | # Collect RPM files matching provided glob. > | rpms='' > | for glog in ${globs}; do > | for file in ${repo}/${glob}; do > | rpms="${rpms} ${file}" > | done > | done > | > | # Verify that at least one RPM file is found. > | if [ -z "${rpms}" ]; then > | <...> > | fi > >But feel free to pick up any way, I'll not push you for mine one. Ok, sure, I've integrated your way with some minor corrections. > > >> >> # prepare the workspace >> prepare_ws ${os}_${option_dist} >> >> # copy the needed package binaries to the workspace >> - cp $repo/*.rpm $ws/. >> + for pack_pattern in $pack_patterns ; do >> + cp $repo/$pack_pattern $ws/. >> + done > >After the change above it can be simplified to: > > | cp ${rpms} "$ws/" > >Note: no quotes around ${rpms} to use file glob expansion. Ok, done. > >> >> pushd $ws >> >> # set the paths >> - if [ "$os" == "el" ]; then >> - repopath=$option_dist/os/x86_64 >> - rpmpath=Packages >> - elif [ "$os" == "fedora" ]; then >> - repopath=releases/$option_dist/Everything/x86_64/os >> - rpmpath=Packages/$proddir >> + repopath=$option_dist/$pack_subdir >> + rpmpath=Packages >> + if [ "$os" == "fedora" ]; then >> + rpmpath=$rpmpath/$proddir >> fi > >I don't think that we need different layout for CentOS and Fedora. >Packagecloud.io has the same flat layout for both. Ok, I've removed extra subdirs and set common paths setup for Fedora and CentOS. > > >> packpath=$repopath/$rpmpath >> >> # prepare local repository with packages >> $mk_dir $packpath >> - mv *.rpm $packpath/. >> + for pack_pattern in $pack_patterns ; do >> + mv $pack_pattern $packpath/. >> + done > >Same as above. Ok, done. > > >> cd $repopath >> >> # copy the current metadata files from S3 >> @@ -547,8 +573,10 @@ EOF >> gpg --detach-sign --armor repodata/repomd.xml >> >> # copy the packages to S3 >> - for file in $rpmpath/*.rpm ; do >> - $aws_cp_public $file "$bucket_path/$repopath/$file" >> + for pack_pattern in $pack_patterns ; do >> + for file in $rpmpath/$pack_pattern ; do >> + $aws_cp_public $file "$bucket_path/$repopath/$file" >> + done > >Same as above. Ok, done. > > >> done >> >> # update the metadata at the S3 >> @@ -563,7 +591,8 @@ EOF >> if [ "$os" == "ubuntu" -o "$os" == "debian" ]; then >> pack_deb >> elif [ "$os" == "el" -o "$os" == "fedora" ]; then >> - pack_rpm >> + pack_rpm x86_64 "*.x86_64.rpm *.noarch.rpm" >> + pack_rpm SRPMS "*.src.rpm" > >It deserves a comment. Added the comment. > > >> else >> echo "USAGE: given OS '$os' is not supported, use any single from the list: $alloss" >> usage >> -- >> 2.17.1 >> > >And while we're here: I would extract explicit keyid to an environment >variable (it is a least in 'SignWith:'). Ok, I've moved it to gitlab-ci variable "GPG_SIGN_KEY" > > >I would also use `gpg -u <...>` when signing, because it is >possible that there is more then one key for a user/machine. Ok, sure, thanks. > -- Alexander Tikhonov