From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergeyb@tarantool.org>
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256
 bits)) (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 56925469719
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 11 Sep 2020 14:10:16 +0300 (MSK)
References: <f2354fe0dd32ee979f91d6c0d8a6e835ce758734.1599047126.git.avtikhon@tarantool.org>
From: Sergey Bronnikov <sergeyb@tarantool.org>
Message-ID: <76e7040b-ea21-0082-b437-27101c0b7b56@tarantool.org>
Date: Fri, 11 Sep 2020 14:10:14 +0300
MIME-Version: 1.0
In-Reply-To: <f2354fe0dd32ee979f91d6c0d8a6e835ce758734.1599047126.git.avtikhon@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Subject: Re: [Tarantool-patches] [PATCH v1] gitlab-ci: save sources to new
 S3 location
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: "Alexander V. Tikhonov" <avtikhon@tarantool.org>, Kirill Yukhin <kyukhin@tarantool.org>, Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Hello, Alexander!

Thanks for the patch. See my 5 comments.

On 02.09.2020 14:46, Alexander V. Tikhonov wrote:
> Changed S3 location for sources tarballs. Also added ability to
> create S3 directory for the tarballs if it was not existed.
> ---
>
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/src-s3-new
>
>   .gitlab.mk | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/.gitlab.mk b/.gitlab.mk
> index 0ce978063..708c69082 100644
> --- a/.gitlab.mk
> +++ b/.gitlab.mk
> @@ -108,6 +108,7 @@ GIT_DESCRIBE=$(shell git describe HEAD)
>   MAJOR_VERSION=$(word 1,$(subst ., ,$(GIT_DESCRIBE)))
>   MINOR_VERSION=$(word 2,$(subst ., ,$(GIT_DESCRIBE)))
>   BUCKET="$(MAJOR_VERSION).$(MINOR_VERSION)"
> +S3_BUCKET="s3://tarantool_repo/sources/$(BUCKET)"
1. Wrong parentheses used around BUCKET.

2. You already have variable BUCKET and add a new variable S3_BUCKET.

For my taste it is difficult to understand what for they need when 
looking on the names.
>   
>   deploy_prepare:
>   	[ -d packpack ] || \
> @@ -130,8 +131,13 @@ source: deploy_prepare
>   	TARBALL_COMPRESSOR=gz packpack/packpack tarball
>   
>   source_deploy: source
> -	aws --endpoint-url "${AWS_S3_ENDPOINT_URL}" s3 cp build/*.tar.gz \
> -		"s3://tarantool.${BUCKET}.src/" --acl public-read
> +	( aws --endpoint-url "${AWS_S3_ENDPOINT_URL}" s3 ls "${S3_BUCKET}/" || \
> +		( rm -rf "${BUCKET}" ; mkdir "${BUCKET}" &&

3. Didn't get a point to use 'ls' operation here.

4. Moreover probably it is better to suppress output from listing 
command as it is not used here.

5. In a command chain above you check existence of remote bucket and 
then remove local directory.

Probably you should check existence of local directory too. Otherwise 
'rm' will fail when directory is not exist.

> +			aws --endpoint-url "${AWS_S3_ENDPOINT_URL}"            \
> +				s3 mv "${BUCKET}" "${S3_BUCKET}" --recursive   \
> +				--acl public-read ) ) &&                       \
> +		aws --endpoint-url "${AWS_S3_ENDPOINT_URL}"                    \
> +			s3 cp build/*.tar.gz "${S3_BUCKET}/" --acl public-read
>   
>   # ###################
>   # Performance testing