Add ldecnumber installation #194
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
avtikhon
force-pushed
the
avtikhon/pr-109-ldecnumber
branch
3 times, most recently
from
905e110 to
11778a0
Compare
avtikhon
force-pushed
the
avtikhon/pr-109-ldecnumber
branch
from
11778a0 to
5c74cd4
Compare
Totktonada
reviewed
Oct 20, 2020
There was a problem hiding this comment.
It seems, we reached the point, where the amount of made work is so that I don't understand anything anymore.
- 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?
- 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?
- We agreed to don't touch existing images, so why we update CI rules for them?
- Why I don't see any CI results in the PR?
- Why all modules are installed by a frozen version, but ldecnumber is installed from master (scm-1)?
- Why all modules are installed by a package name + version, but ldecnumber by a rockspec URL?
- 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] - #109 (comment) |
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] - #109 Co-authored-by: Alexander V. Tikhonov <[email protected]>
avtikhon
force-pushed
the
avtikhon/pr-109-ldecnumber
branch
from
5c74cd4 to
f2a6532
Compare
avtikhon
force-pushed
the
master
branch
3 times, most recently
from
0502fff to
b43ea3a
Compare
avtikhon
force-pushed
the
master
branch
2 times, most recently
from
8127002 to
75c890f
Compare
Closing then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current PR based on #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] - #109
Co-authored-by: Alexander V. Tikhonov [email protected]