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

[fix] checking for compatible redis-server version. #395

Merged
merged 4 commits into from
May 11, 2020
Merged

[fix] checking for compatible redis-server version. #395

merged 4 commits into from
May 11, 2020

Conversation

filipecosta90
Copy link
Contributor

  • logging redistimeseries module version, and git_sha.
  • checking for compatible redis-server version ( at least 5.0.0 given the RedisModuleDict ) cc @K-Jo
  • using same code as RedisGears to check for redis-server version and setup

fixes #392
fixes #393
fixes #394

@codecov
Copy link

codecov bot commented May 8, 2020 •
edited
Loading

Codecov Report

Merging #395 into master will decrease coverage by 0.62%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   95.34%   94.71%   -0.63%     
==========================================
  Files          11       12       +1     
  Lines        1760     1799      +39     
==========================================
+ Hits         1678     1704      +26     
- Misses         82       95      +13
Impacted Files Coverage Δ
src/config.c 80.76% <65.51%> (-19.24%) ⬇️
src/module.c 93.83% <66.66%> (-0.39%) ⬇️
src/config.h 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94aa278...f4e4e30. Read the comment docs.

@ashtul
Copy link
Contributor

ashtul commented May 9, 2020

@filipecosta90
Maybe you can try and get this into RedisModule API as well? It will lack back compatibility but will be useful in the future.

MeirShpilraien
MeirShpilraien reviewed May 9, 2020
View reviewed changes
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

Some more namespaces

src/config.c Outdated Show resolved Hide resolved
src/config.c Outdated Show resolved Hide resolved
src/config.h Outdated Show resolved Hide resolved
src/config.h Outdated Show resolved Hide resolved
@filipecosta90
Copy link
Contributor Author

@ashtul @MeirShpilraien can you guys please revise. prefixed all functions and vars with RTS_*.

MeirShpilraien
MeirShpilraien reviewed May 9, 2020
View reviewed changes
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

@filipecosta90 it looks good to me, just make sure with @ashtul / @danni-m if its OK to put it under config.c

danni-m
danni-m reviewed May 9, 2020
View reviewed changes
src/config.c Outdated Show resolved Hide resolved
danni-m
danni-m reviewed May 9, 2020
View reviewed changes
src/config.c Outdated Show resolved Hide resolved
@filipecosta90 filipecosta90 requested a review from danni-m May 9, 2020 22:29
@K-Jo
Copy link
Collaborator

K-Jo commented May 11, 2020

@filipecosta90 shouldn't it be in the ramp.yaml file as well?

@filipecosta90
Copy link
Contributor Author

@filipecosta90 shouldn't it be in the ramp.yaml file as well?

as @danni-m stated, already there :)
https://github.com/RedisTimeSeries/RedisTimeSeries/blob/master/ramp.yml#L8

@filipecosta90 filipecosta90 merged commit ed0a4d7 into master May 11, 2020
@filipecosta90 filipecosta90 deleted the version.verify branch May 11, 2020 09:15
ashtul pushed a commit that referenced this pull request May 21, 2020
@filipecosta90 @ashtul
[fix] checking for compatible redis-server version. (#395)
4675806 
* [fix] checking for compatible redis-server version. logging git_sha. using same approach as RedisGears

Co-authored-by: Ariel Shtul <ariel.shtul@redislabs.com>
@ashtul ashtul mentioned this pull request May 21, 2020
Merged
ashtul pushed a commit that referenced this pull request May 21, 2020
1.2.6 cherry pick (#407)
d581245 
* fix issue 373 (#374)

* Fix makefile failure before submodules init (#386)

* [fix] checking for compatible redis-server version.  (#395)

* copy fix from queryindex to mrange & mget + test (#403)

* tests for issues 391 & 400

* modification to PR 293 (#336)

* Redis 6.0.1 in builders (#388)

* use snprints instead of ReplyWithDouble
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danni-m danni-m danni-m approved these changes

@ashtul ashtul Awaiting requested review from ashtul

@MeirShpilraien MeirShpilraien Awaiting requested review from MeirShpilraien

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@filipecosta90 @ashtul @K-Jo @danni-m @MeirShpilraien