search

tarantool / docker

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

Add ldecnumber installation #194

Closed avtikhon closed 2 years ago

avtikhon commented 3 years ago

Current PR based on https://github.com/tarantool/docker/pull/109

According to PR-109 [1] it was decided to add ldecnumber installation. Depending that 2.x images are lack of decimals support for now checked Tarantool releases and tags that need to be updated with it. Found that Tarantool since 2.2 release branches and 2.2.1 release tags are lack of it, than ldecnumber number installation was added to the earlier tags and release branches.

Closes #77 Closes #112

[1] - https://github.com/tarantool/docker/pull/109

Co-authored-by: Alexander V. Tikhonov avtikhon@tarantool.org

avtikhon commented 3 years ago

It seems, we reached the point, where the amount of made work is so that I don't understand anything anymore.

  1. AFAIR, we agreed to don't add new modules, but rather give a recipe how to do so in child images. Why we add the new one?
  2. Why presence of this module is controlled by variables in the CI and don't depend of a tarantool version? Why don't verify a tarantool version right inside a docker file?
  3. We agreed to don't touch existing images, so why we update CI rules for them?
  4. Why I don't see any CI results in the PR?
  5. Why all modules are installed by a frozen version, but ldecnumber is installed from master (scm-1)?
  6. Why all modules are installed by a package name + version, but ldecnumber by a rockspec URL?
  7. How it ever work with almost-vanilla luarocks if, say, avro-schema is present only on rocks.tarantool.org server?

I really unable to review without understanding. Sorry.

  1. Let's discuss the recipe we should create and how it should look like. Also need to understand how to post the information on it.
  2. Seems to have checks inside the dockerfiles for some Tarantool versions is bad than have it in common place - configuration at yaml.
  3. Right, I've just used it to provide the new variable. This gitlab-ci doesn't push the images and we can set any needed name.
  4. As I warned before, there is some issue with mirroring on gitlab-ci, after the pay to gitlab was stopped.
  5. Pull-request #109 was agreed and it was already discussed in [1], also check that master was used in original pull-request [2].
  6. Commit in pull-request used master from rockspec URL [2].
  7. Need more information on it.

[1] - https://github.com/tarantool/docker/pull/109#issuecomment-502548609 [2] - https://github.com/tarantool/docker/pull/109/commits/b23cc9e5eecf1bf8508a57b936328daec74cd32a

kyukhin commented 2 years ago

It seems, we reached the point, where the amount of made work is so that I don't understand anything anymore.

  1. AFAIR, we agreed to don't add new modules, but rather give a recipe how to do so in child images. Why we add the new one?
  2. Why presence of this module is controlled by variables in the CI and don't depend of a tarantool version? Why don't verify a tarantool version right inside a docker file?
  3. We agreed to don't touch existing images, so why we update CI rules for them?
  4. Why I don't see any CI results in the PR?
  5. Why all modules are installed by a frozen version, but ldecnumber is installed from master (scm-1)?
  6. Why all modules are installed by a package name + version, but ldecnumber by a rockspec URL?
  7. How it ever work with almost-vanilla luarocks if, say, avro-schema is present only on rocks.tarantool.org server?

I really unable to review without understanding. Sorry.

Closing then.