This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add redis SSL configuration options #15312
Add redis SSL configuration options #15312
Changes from 6 commits
2cfd40e
401af4e
29bc340
66f9b8f
873c3c0
3e4faf2
9deb410
4411ee3
55cc759
2392bfd
a4c82c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm guessing this refers to the
dbid
option specifically?We should have explicitly point out when the new options were added.
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.
ah yes, that's a good point, I didn't spot that since it was floating.
I guess it would be sensible to put
_Added in Synapse 1.78.0._
on the original options and add (I believe)_Added in Synapse 1.83.0._
on the new ones.Otherwise might be worth seeing what the other option groups do if the 'sub-options' were added at different times.
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.
If in doubt, my advice would be to copy what the Python docs do, e.g. https://docs.python.org/3/library/subprocess.html?highlight=subprocess#subprocess.TimeoutExpired
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.
I added the changelog with version. I used version 1.84.0, since 1.83.0 is already in RC so I assume it won't end up in that one.
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.
I'm slightly suspicious of
Context
s because I remember the defaults being a bit iffy.Before merging, we should check the correct TLS verification flags are set on this.
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.
(I looked into this and it seems sane. Notably, it's the Twisted SSL context, not the vanilla Python one 😵💫.)