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

Allow CheckCertificateRevocation to be set on connection string #1591

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

lwlwalker
Copy link
Contributor

This configuration setting can't be set right now using connection string.
Usage example:
"redishost:6380,ssl=True,checkCertificateRevocation=false"

Copy link
Collaborator

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

Missing inclusion when generating a string - see public string ToString(bool includePassword) - and should probably be a test for that (perhaps just test the output of options.ToString() in ConfigurationOption_CheckCertificateRevocation?) - but otherwise: looks great, thanks!

@lwlwalker
Copy link
Contributor Author

ToString functionality added and unit-tested

@mgravell
Copy link
Collaborator

Looks fine, thanks. I'll pull it locally before merging, but: 👍

@NickCraver
Copy link
Collaborator

Merging main in here to get build checks decent

@lwlwalker
Copy link
Contributor Author

Hi @mgravell , @NickCraver ,

Any news on this? I don't know why the build fails, as my changes are not related to the errors there (I think 8-D), and I created my Fork from master...

Please, let me know if I can help here somehow...

@mgravell
Copy link
Collaborator

mgravell commented Nov 2, 2020

@lwlwalker I'm not concerned about the unrelated build failures - I just haven't had time to revisit for a build/merge session

@mgravell mgravell merged commit a81d902 into StackExchange:main Nov 13, 2020
@mgravell
Copy link
Collaborator

merged with thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants