Re: [Tarantool-patches] [PATCH v1] gitlab-ci: save sources to new S3 location

[Sergey Bronnikov <sergeyb@tarantool.org>]

LGTM

On 11.09.2020 15:54, Alexander V. Tikhonov wrote:
> Hi Sergey, thanks for the review, pplease check my comments below.
>
> On Fri, Sep 11, 2020 at 02:10:14PM +0300, Sergey Bronnikov wrote:
>> 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.
>>
> That's ok for the current string, due to it depends on Makefile rules.
>
>> 2. You already have variable BUCKET and add a new variable S3_BUCKET.
>>
> BUCKET is a simple bucket name;
> S3_BUCKET renamed to S3_BUCKET_URL to make it more informative.
>
>> 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.
>>
> AWS S3 tools too weak and 'ls' used there to check the existence of the
> directory.
>
>> 4. Moreover probably it is better to suppress output from listing command as
>> it is not used here.
>>
> Here it is really needed to save in output the result.
>
>> 5. In a command chain above you check existence of remote bucket and then
>> remove local directory.
>>
> Unfortunately AWS S3 doesn't have any command to create the directory.
> The only way to create it is to copy recursively the empty one. So the
> local empty directory is needed for it.
>
>> 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