search

tarantool / docker

Docker images for tarantool database
https://hub.docker.com/r/tarantool/tarantool
52 stars 25 forks source link

Merge alpine_3.9* dockerfiles #178

Closed avtikhon closed 4 years ago

avtikhon commented 4 years ago

Created 9 commits to make dockerfiles/alpine3.9*.x files looks the same and merged it into single common file dockerfiles/alpine_3.9.

LeonidVas commented 4 years ago

Hi! Thank you for the patch. I have some comments / questions:

avtikhon commented 4 years ago

Thanks, for the review, please check my comments below:

Hi! Thank you for the patch. I have some comments / questions:

  • Please, split it into two commits (merge dockerfiles and add LUAROCKS_INSTALL var.)

Not really possible, due to:

  • I find it strage that we want to use a luarocks instead of tarantoolctl rocks. What is it for? Who is the customer?

In real we need to use 'tarantooctl rocks' command to install rocks, but not all of the Tarantool versions has it workable - in this way we need to use 'luarocks' as workaround.

LeonidVas commented 4 years ago

Not really possible, due to:

  • merged dockerfiles would have 'tarantoolctl rocks' either 'luarocks' only w/o LUAROCKS_INSTALL variable and some of the builds would fail;
  • sure the implementation of the LUAROCKS_INSTALL variable as the first commit would fix the issue mentioned above, but in this way the feature will be implemented w/o it's use - dead code. If this solution is ok, I'll do it.

Already discussed, just for a note: "1st commit - add var, 2nd - merge".

LeonidVas commented 4 years ago

In real we need to use 'tarantooctl rocks' command to install rocks, but not all of the Tarantool versions has it workable - in this way we need to use 'luarocks' as workaround.

Ok. I am surprised.

avtikhon commented 4 years ago

Not really possible, due to:

  • merged dockerfiles would have 'tarantoolctl rocks' either 'luarocks' only w/o LUAROCKS_INSTALL variable and some of the builds would fail;
  • sure the implementation of the LUAROCKS_INSTALL variable as the first commit would fix the issue mentioned above, but in this way the feature will be implemented w/o it's use - dead code. If this solution is ok, I'll do it.

Already discussed, just for a note: "1st commit - add var, 2nd - merge".

Right, but looking deeply found that all the changes were better to split.

avtikhon commented 4 years ago

In real we need to use 'tarantooctl rocks' command to install rocks, but not all of the Tarantool versions has it workable - in this way we need to use 'luarocks' as workaround.

Ok. I am surprised.

Unfortunately yes.

LeonidVas commented 4 years ago

Not really possible, due to:

  • merged dockerfiles would have 'tarantoolctl rocks' either 'luarocks' only w/o LUAROCKS_INSTALL variable and some of the builds would fail;
  • sure the implementation of the LUAROCKS_INSTALL variable as the first commit would fix the issue mentioned above, but in this way the feature will be implemented w/o it's use - dead code. If this solution is ok, I'll do it.

Already discussed, just for a note: "1st commit - add var, 2nd - merge".

Right, but looking deeply found that all the changes were better to split.

Great job! See the comments per commit below:

LeonidVas commented 4 years ago
  • "Set rocks tool installer using variable" - may be add information to the commit message that the commit is a preparation for the merge of dockerfiles? Up to you

LGTM, after split commit.

avtikhon commented 4 years ago

Not really possible, due to:

  • merged dockerfiles would have 'tarantoolctl rocks' either 'luarocks' only w/o LUAROCKS_INSTALL variable and some of the builds would fail;
  • sure the implementation of the LUAROCKS_INSTALL variable as the first commit would fix the issue mentioned above, but in this way the feature will be implemented w/o it's use - dead code. If this solution is ok, I'll do it.

Already discussed, just for a note: "1st commit - add var, 2nd - merge".

Right, but looking deeply found that all the changes were better to split.

Great job! See the comments per commit below:

  • "Set rocks tool installer using variable" - may be add information to the commit message that the commit is a preparation for the merge of dockerfiles? Up to you.

Seems not really needed, due to merge is not the cause of the change, decided to add more info.

  • "Use LUAROCKS_INSTALL for dockerfiles builds" - (Part of ?) LGTM.
  • "Use LUAROCKS_INSTALL for rest of dockerfiles" - LGTM.
  • "Add server path for luarocks ldoc installation" - (Part of ?). See a question in the code.

Patch removed.

  • "Enable ENABLE_BUNDLED_LIBYAML flag" - LGTM.
  • "Enable manual build with makefile" - local host -> localhost? How can I run this? Can I do it locally?

Right, in previous to rerun the build too many variables had to be manually took from gitlab-ci yaml configuration. After this patch make file can be run manually just with minimal number of variables.

  • "Remove perl installation from build depends" - Why Perl has become obsolete? Are you disappointed in Perl?:-)

Perl removed from build time installation part, it means that it will be removed just after the build anyway.

  • "Add unzip tool to rocks installations" - LGTM
  • "Use libressl in Alpine based images builds" - the policy of the tarantool team - to use an openssl library instead of libressl. Have you decided to move against the current? You must have a good reasoned position. Please, list all your arguments.

Decided to use openssl instead of libressl.

"Merge same Alpine 3.9 dockerfiles" - LGTM

LeonidVas commented 4 years ago

LGTM. As I understand it, we need to push # 177 to get the green pipeline in gitlab. Please, make sure the build works correctly in gitlab before pushing this patchset.