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

Refactor redis setup #42

Merged
merged 2 commits into from Jul 23, 2018
Merged

Refactor redis setup #42

merged 2 commits into from Jul 23, 2018

Conversation

armenzg
Copy link
Contributor

@armenzg armenzg commented Jul 18, 2018

For some reason, redis was not working for me locally.
Upon further research I noticed that then-redis has been depecrated
and we can use async-redis to help using async/await.

I refactored the fetchText.js module to separate the logic when
using Redis and when not.

@armenzg armenzg added the enhancement New feature or request label Jul 18, 2018
@armenzg armenzg self-assigned this Jul 18, 2018
@armenzg armenzg requested a review from edmorley July 18, 2018 14:13
@armenzg armenzg temporarily deployed to firefox-health-backend-pr-42 July 18, 2018 14:13 Inactive
@coveralls
Copy link

coveralls commented Jul 18, 2018 •
edited

Coverage Status

Coverage decreased (-0.3%) to 35.885% when pulling d4edb5a on redis_refactor into d06327e on master.

@armenzg armenzg requested a deployment to firefox-health-backend-pr-42 July 18, 2018 14:16 Abandoned
@armenzg
Copy link
Contributor Author

armenzg commented Jul 18, 2018

I've tested the deployment by loading some URLs and see if the Redis addon reports an increase of the storage.
e.g:
https://firefox-health-backend-pr-42.herokuapp.com/api/android/nimbledroid?product=focus

@armenzg armenzg requested a review from eliperelman July 20, 2018 12:35
edmorley
edmorley reviewed Jul 23, 2018
View reviewed changes
Copy link

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

I'm not familiar with any of the mentioned libraries - however using a non-deprecated one sounds like a good idea. The PR seems fine at a quick glance, but I'm not familiar with the app, the libraries etc.

@armenzg
Refactor redis setup
1f870a9 
For some reason, redis was not working for me locally.
Upon further research I noticed that `then-redis` has been depecrated
and we can use `async-redis` to help using async/await.

I refactored the fetchText.js module to separate the logic when
using Redis and when not.
@armenzg
Review apps should add the heroku-redis addon
f7915e2
@armenzg armenzg merged commit 205bdb0 into master Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmorley edmorley edmorley left review comments

@eliperelman eliperelman Awaiting requested review from eliperelman

Assignees

@armenzg armenzg

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@armenzg @coveralls @edmorley