Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add curl for rocks installations #179

Closed
wants to merge 2 commits into from
Closed

Add curl for rocks installations #179

wants to merge 2 commits into from

Conversation

avtikhon
Copy link

@avtikhon avtikhon commented Jul 13, 2020 •
edited
Loading

Added curl and git packages for run time to be able to install rocks
without additional dependences.

Also added curl-dev package to build time of the image. It helped to
avoid of local build of the curl from sources of version 5.59.0 due
to alpine OS has the following curl default packages:
alpine 3.5: curl 7.61.1-r1
alpine 3.9: curl 7.64.0-r3

After build for curl from sources became unneeded, the dockerfiles
alpine_3.5_2.x and alpine_3.9 became the same and were merged.

All builds for Tarantool 2.x except 2.1.0 based on alpine 3.5 version
moved to use 3.9 version.

Removed installation of tarantool_curl rock from alpine_3.5_1.x
dockerfile, so alpine_3.5_* dockerfiles became the same and were
merged into alpine_3.5.

Closes #168
Part of #152

@avtikhon avtikhon requested a review from Totktonada July 13, 2020 08:36
@avtikhon avtikhon self-assigned this Jul 13, 2020
@avtikhon avtikhon force-pushed the avtikhon/gh-168 branch 10 times, most recently from 4279f1b to 96954ab Compare July 15, 2020 08:50
LeonidVas
LeonidVas reviewed Jul 29, 2020
View reviewed changes
Copy link

@LeonidVas LeonidVas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thank you for the patch.
For 1d3083b
See comment #178 (comment)

LeonidVas
LeonidVas reviewed Jul 29, 2020
View reviewed changes
Copy link

@LeonidVas LeonidVas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thank you for the patch.
For 96954ab
Please, separate it into few commits (approximately the same as in your comment message.)

README.md Outdated
@@ -310,7 +310,7 @@ A maintaner is responsible for merging the PR.
Say, we have updated dockerfiles/alpine_3.9_2.x and want to check it:

```sh
$ IMAGE=tarantool/tarantool TAG=2 OS=alpine DIST=3.9 VER=2.x DOCKERFILE_NAME_SUFFIX=2.x \
$ IMAGE=tarantool/tarantool TAG=2 OS=alpine DIST=3.9 VER=2.x DOCKERFILE_NAME_SUFFIX= \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just remove "DOCKERFILE_NAME_SUFFIX="?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it could be removed, but due to README file gives some usage explanation and not real code, I wanted to save it there as the format of the possible use.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already discussed, just for a note: "This can be saved in the README, but not in this view."

@avtikhon
Copy link
Author

Hi! Thank you for the patch.
For 96954ab
Please, separate it into few commits (approximately the same as in your comment message.)

Ok, I'll split it a bit, but do we really want to split too much ? I'm asking it because I think that swapping/changing curl must be in a single commit. So I would suggest the following splitting on commits:

  1. Curl installation was changed from source curl to default in the OS. But changing it we needed curl-dev too not to break the builds, so it must be in a single commit:
Added curl and git packages for run time to be able to install rocks
without additional dependences.

Also added curl-dev package to build time of the image. It helped to
avoid of local build of the curl from sources of version 5.59.0 due
to alpine OS has the following curl default packages:
alpine 3.5: curl 7.61.1-r1
alpine 3.9: curl 7.64.0-r3
  1. Do we really need to split it from the 1st commit, as these was produced but changing curl and no other changes made for it ?
After build for curl from sources became unneeded, the dockerfiles
alpine_3.5_2.x and alpine_3.9 became the same and were merged.

All builds for Tarantool 2.x except 2.1.0 based on alpine 3.5 version
moved to use 3.9 version.
  1. The following part of commit can be really split into standalone commit, but as in previous comment, does it need to be splitted into single commit or two ?
Removed installation of tarantool_curl rock from alpine_3.5_1.x
dockerfile, so alpine_3.5_* dockerfiles became the same and were
merged into alpine_3.5.

@avtikhon avtikhon requested a review from LeonidVas July 31, 2020 03:15
@avtikhon
Copy link
Author

Hi! Thank you for the patch.
For 1d3083b
See comment #178 (comment)

Commented in there, please check.

@LeonidVas
Copy link

LeonidVas commented Aug 5, 2020 •
edited
Loading

Hi! Thank you for the patch.
For 96954ab
Please, separate it into few commits (approximately the same as in your comment message.)

Ok, I'll split it a bit, but do we really want to split too much ? I'm asking it because I think that swapping/changing curl must be in a single commit. So I would suggest the following splitting on commits:

  1. Curl installation was changed from source curl to default in the OS. But changing it we needed curl-dev too not to break the builds, so it must be in a single commit:
Added curl and git packages for run time to be able to install rocks
without additional dependences.

Also added curl-dev package to build time of the image. It helped to
avoid of local build of the curl from sources of version 5.59.0 due
to alpine OS has the following curl default packages:
alpine 3.5: curl 7.61.1-r1
alpine 3.9: curl 7.64.0-r3
  1. Do we really need to split it from the 1st commit, as these was produced but changing curl and no other changes made for it ?
After build for curl from sources became unneeded, the dockerfiles
alpine_3.5_2.x and alpine_3.9 became the same and were merged.

All builds for Tarantool 2.x except 2.1.0 based on alpine 3.5 version
moved to use 3.9 version.
  1. The following part of commit can be really split into standalone commit, but as in previous comment, does it need to be splitted into single commit or two ?
Removed installation of tarantool_curl rock from alpine_3.5_1.x
dockerfile, so alpine_3.5_* dockerfiles became the same and were
merged into alpine_3.5.

Already discussed, just for a note: "You've done a nice splitting of commits at #178. I think you can repeat the same here."

@tsafin says #177 should fix the problem.

avtikhon added 2 commits August 6, 2020 18:26
@avtikhon
Add curl for rocks installations
cd15edb 
Added curl and git packages for run time to be able to install rocks
without additional dependences.

Also added curl-dev package to build time of the image. It helped to
avoid of local build of the curl from sources of version 5.59.0 due
to alpine OS has the following curl default packages:
alpine 3.5: curl 7.61.1-r1
alpine 3.9: curl 7.64.0-r3

Removed installation of tarantool_curl rock from alpine_3.5_1.x
dockerfile, so alpine_3.5_* dockerfiles became the same and were
merged into alpine_3.5.

Closes #168
Part of #152
@avtikhon
Bump Alpine version to 3.9
1452e83 
Bumped Alpine version from 3.5 to 3.9 for building
Tarantool versions:

 - 2.1.1
 - 2.1.2
 - 2.2.0
 - 2.2.1
 - 2.3.0

Part of #152
@LeonidVas LeonidVas removed their request for review October 19, 2020 11:31
@avtikhon avtikhon force-pushed the master branch 3 times, most recently from 0502fff to b43ea3a Compare October 25, 2020 13:49
@avtikhon avtikhon force-pushed the master branch 2 times, most recently from 8127002 to 75c890f Compare April 22, 2021 20:05
@Totktonada Totktonada requested a review from ylobankov December 8, 2021 03:40
@Totktonada Totktonada removed their request for review December 15, 2021 09:24
@VitaliyaIoffe
Copy link

I rebased it and it seems as doesn't work now

@sergos
Copy link

sergos commented Nov 18, 2022

@ylobankov please proceed with review/assign

@kyukhin
Copy link

kyukhin commented Jun 30, 2023

Alpine is going to be abandoned.

@kyukhin kyukhin closed this Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeonidVas LeonidVas LeonidVas left review comments

@ylobankov ylobankov Awaiting requested review from ylobankov

Assignees

@NickVolynkin NickVolynkin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

docker: Unable to install rock from a fresh image
6 participants
@avtikhon @LeonidVas @VitaliyaIoffe @sergos @kyukhin @NickVolynkin