Unblock redis>=4.0.2 (#542, #570)

[hartwork]

Fixes #542

Upstream commit redis/redis-py@d2b2333 that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue #542, follow-up to pull request #570

[hartwork]

Any thoughts?

[WisdomPill]

Sorry I away from my laptop, will have a look at it by the end of weekend

[codecov]

Codecov Report

Merging #576 (00e0d13) into master (c7c3ab3) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #576   +/-   ##
======================================
  Coverage    57.7%   57.7%           
======================================
  Files          39      39           
  Lines        2497    2497           
  Branches       63      63           
======================================
  Hits         1440    1440           
  Misses       1049    1049           
  Partials        8       8           

Flag	Coverage Δ
mypy	33.8% <ø> (ø)
tests	83.3% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

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 c7c3ab3...00e0d13. Read the comment docs.

[hartwork]

@WisdomPill thanks!

[WisdomPill]

It is okay for me, but now I have some doubts about the matrix strategy, I think we should test with specific versions of redis-py and maybe also specific versions of redis server.

By the way there is a comment in the ci action which should be uncommented, like here.

@terencehonles what do you think?

[terencehonles]

@WisdomPill I pushed the commit from this PR to #575 because I both have permission to push to that branch, but also it is the older branch. The change seems fine to me, but I will not have a chance to test it for a few days because I'm flying now and will likely be busy for a bit. I don't know why this should be an issue since this should return the previous functionality back to how it was before. I do think there are some issues with the connection pools Redis configures since now there is both a sentinel pool and a redis pool, but that should have already been an issue previously and this change should likely be sufficient until we have a more pressing need to allow independent connection pool configuration.

[WisdomPill]

Okay, let's finish this on #575 then, but I see that there is an issue with packaging and there is already an issue open in redis/redis-py#1784 about it, so I guess we can wait to see what happens, by the way @hartwork is already involved in that conversation.

[hartwork]

By the way there is a comment in the ci action which should be uncommented, like here.

@WisdomPill I had to keep those commented because redis-py master is broken in that regard, which I know you know by now. While I personally like the idea of becoming aware of future issues early, unless the CI gets an "accept failure" flag for those moving targets, moving targets are by design a problem to CI stability and reproducibility, so maybe not testing against redis-py master is in the interest of everyone working on pull requests around, for now.

[WisdomPill]

I see your point, but I think keeping master on the CI and ignoring it when there are known issues is vital,
especially for the people maintaining those libraries,
in our case if we did not know that redis-py 4 was breaking sentinel support then our users would have discovered it in a painful way.
I see it as our duty to inform maintainers of libraries we use that the next version has something wrong that should be addressed before releasing on our or their side.
Even if it breaks and stays broken for sometime, we can always remove it and restore it.

[hartwork]

I see your point, but I think keeping master on the CI and ignoring it when there are known issues is vital, especially for the people maintaining those libraries

Yes, but as long as any single CI task fails the whole thing will marked as failed and then people learn that CI is often red and always needs extra checking.

I see it as our duty to inform maintainers of libraries we use that the next version has something wrong that should be addressed before releasing on our or their side. Even if it breaks and stays broken for sometime, we can always remove it and restore it.

I see your point, but personally I think waiting for future breakage and bug reports and a fully green CI is more healthy. I guess it's okay that we disagree on that, it's just my 2 cents.