-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fixes #3984: Allow for Redis Sentinel Connection Configuration #4161
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
Fixes #3984: Allow for Redis Sentinel Connection Configuration #4161
Conversation
One quick comment, we should perhaps not have the docs under requirements.md. Maybe in a separate sentinel configuration doc or something (will wait for @jeremystretch for this), I just think if we include this in requirements it will be viewed as a requirement. |
I thought about the location for the docs a little, and I decided to place them under the initial Redis docs since 1) using sentinel for redis does not change the fact that it is required and 2) it reduces searching around the documentation if someone needs to use Sentinel from the get-go I will fix the linting error (as well as need to update the connection check in the |
Hold off on this until #4159 is fixed please as it will introduce a conflict. |
netbox/netbox/settings.py
Outdated
REDIS_CACHE_CON_STRING = 'rediss://' | ||
if CACHING_REDIS_USING_SENTINEL: | ||
CACHEOPS_SENTINEL = { | ||
'locations': CACHING_REDIS_SENTINELS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line appears to be throwing the PEP8 error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also fix up the PEP8 error?
Oops, I missed you comment about holding off...I will remove that last commit and wait for the merge of #4159. I'll fix the linting errors once I rebase and remove any conflicts |
b24e42d
to
72f0e31
Compare
Alright @jeremystretch @DanSheps this should be good to go now, thanks for getting the other one in so quickly |
I've asked @lampwins to sanity-check this as well as he's probably most familiar with the caching configuration. |
That's fine. For more context, I am currently running with the same settings changes in our production instance(s) and we haven't had any problems |
In addition to.that, I do think the docs for this particular piece should go in the optional settings doc file. Additionally, not sure how I feel about the sentinel config in the example config. |
Yea, I mean, it doesn't matter too much to me as long as its somewhere, so I don't mind moving it. I'll update it :) |
We can leave it as is, I was just worried some people might see it and think it is something that is required |
I guess I'll leave it as is unless anyone has a strong opinion one way or another. |
[Corollary to #4161] Redis Connection Check when Using Sentinel
Fixes: #3984
Adds functionality and documentation to allow Redis for both the webhook and caching layers to use Redis Sentinel.