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

Taking redis client from django.core.cache #82

Merged
merged 11 commits into from
Jun 24, 2017
Merged

Taking redis client from django.core.cache #82

merged 11 commits into from
Jun 24, 2017

Conversation

Franr
Copy link
Contributor

@Franr Franr commented Jun 22, 2017

The idea is to avoid rewriting the Redis URL on the defender settings, since probably you already have it on your django CACHES settings.

Pending:

  • tests
  • documentation

@kencochrane
Copy link
Collaborator

django 1.6 is no longer supported, support ended on April 1, 2015 https://www.djangoproject.com/download/#supported-versions did you mean another version?

@@ -18,6 +19,8 @@ def get_redis_connection():
""" Get the redis connection if not using mock """
if config.MOCK_REDIS: # pragma: no cover
return MOCKED_REDIS # pragma: no cover
elif config.DEFENDER_REDIS_NAME: # pragma: no cover
return caches[config.DEFENDER_REDIS_NAME].get_master_client()
Copy link
Collaborator

Choose a reason for hiding this comment

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

we talked about this in the issue #81 instead of adding the and to the elif, maybe we do a check above, and then display an error message and fail if the DEFENDER_REDIS_NAME is set but not available in caches. This way we still fail right away, and we give it a nice error message to let them know what is going on vs just a generic KeyError.

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.4%) to 95.437% when pulling aa40dea on Franr:redis-from-cache into a59cbca on kencochrane:master.

@Franr
Copy link
Contributor Author

Franr commented Jun 22, 2017

Well no, but travis-ci is still running tests over it, so I thought you still wanted to support it. Also it is still on the doc https://github.com/kencochrane/django-defender#requirements 😛

@kencochrane
Copy link
Collaborator

ha, yeah we should probably clean that up and remove support below django 1.8.

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.4%) to 95.437% when pulling 32c20d7 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@Franr
Copy link
Contributor Author

Franr commented Jun 22, 2017

Good. @kencochrane removed support for both versions 👍

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.4%) to 95.437% when pulling 5cef219 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@kencochrane
Copy link
Collaborator

thank you

@coveralls
Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.4%) to 95.437% when pulling 5cef219 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-0.3%) to 95.49% when pulling 2727f63 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage increased (+0.03%) to 95.85% when pulling ec6c3f1 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage increased (+0.03%) to 95.85% when pulling 9244de1 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage increased (+0.03%) to 95.85% when pulling d569a33 on Franr:redis-from-cache into a59cbca on kencochrane:master.

@MattBlack85
Copy link
Contributor

MattBlack85 commented Jun 24, 2017

one note here, since we are dropping support for django < 1.8 we should remove python 2.6 as well from the test matrix since from django 1.8 only python 2.7 is supported

https://docs.djangoproject.com/en/1.11/faq/install/#what-python-version-can-i-use-with-django

@kencochrane
Copy link
Collaborator

@MattBlack85 Good catch, let's do that as well

@coveralls
Copy link

coveralls commented Jun 24, 2017

Coverage Status

Coverage increased (+0.03%) to 95.85% when pulling b05d74b on Franr:redis-from-cache into a59cbca on kencochrane:master.

@Franr
Copy link
Contributor Author

Franr commented Jun 24, 2017

@kencochrane @MattBlack85 done, and added a little doc about the new setting. Who should I ask for a review?

Copy link
Collaborator

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@kencochrane kencochrane merged commit d2b712e into jazzband:master Jun 24, 2017
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.

4 participants