[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