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

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

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