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