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.

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

fix "falsy" arguments in TS.ALTER #2483

Merged
merged 1 commit into from Apr 26, 2023
Merged

fix "falsy" arguments in TS.ALTER #2483

merged 1 commit into from Apr 26, 2023

Conversation

leibale
Copy link
Collaborator

@leibale leibale commented Apr 26, 2023

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ba31c8a) 95.66% compared to head (f1c85a8) 95.66%.

❗ Current head f1c85a8 differs from pull request most recent head 7a117de. Consider uploading reports for the commit 7a117de to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2483   +/-   ##
=======================================
  Coverage   95.66%   95.66%           
=======================================
  Files         455      455           
  Lines        4548     4548           
  Branches      520      520           
=======================================
  Hits         4351     4351           
  Misses        128      128           
  Partials       69       69           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leibale leibale merged commit 986a510 into master Apr 26, 2023
18 checks passed
@leibale leibale deleted the fix-2481 branch April 26, 2023 18:29
@MR4online
Copy link

@leibale thank you for the quick fix and merge! Unfortunately I only now get to test and it doesn't seem to work.
I see your code changes in the typescript files of this repo. All fine.
But I don't see the code changes in the javascript files of the current npm redis version 4.6.7

The condition in the time-series index.js file still says:
if (retention) { args.push('RETENTION', retention.toString()); }

May you please have a look, again?
Thank you very much!

@leibale
Copy link
Collaborator Author

leibale commented Aug 7, 2023

@MR4online looks like @redis/time-series was not released since this fix (although it should have)... I'll release a new version "soon"...

BTW, in the meantime you can use .sendCommand directly to workaround that:

client.sendCommand(['TS.ALTER', '...']);

@leibale
Copy link
Collaborator Author

leibale commented Aug 23, 2023

@MR4online redis@4.6.8/@redis/time-series@1.0.5 are on npm 🎉

@MR4online
Copy link

@leibale thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS.ALTER does not update retention
3 participants