[Tarantool-patches] [PATCH v1] Make S3 paths the same to Tarantool documented
Alexander Tikhonov
avtikhon at tarantool.org
Wed Feb 19 13:53:15 MSK 2020
Alexander, thanks a lot for a deep review, I've answered the questions below.
>Вторник, 18 февраля 2020, 20:08 +03:00 от Alexander Turenko <alexander.turenko at tarantool.org>:
>
>> 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.
> |
> | <everything else you change>
> |
> | 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 <keyid> <...>` when signing, because it is
>possible that there is more then one key for a user/machine.
Ok, sure, thanks.
>
--
Alexander Tikhonov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200219/333292ef/attachment.html>
More information about the Tarantool-patches
mailing list