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

Add missing ssl related arguments to Redis from_url method #12166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonathan-h-grebe
Copy link

@jonathan-h-grebe jonathan-h-grebe commented Jun 19, 2024

solves #12149, the gist of which is:

  • a Redis client (redis.Redis) can be obtained by using either its constructor, or from_url method
  • connection options can be specified as arguments to either
  • some of those options (eg ssl_ca_data) were missing as defined parameters for from_url
  • this caused mypy to give false positives those missing SSL options were specified as kwargs to from_url

Change Details

To fix the issue, following 9 options were added as params to from_url

  • ssl_ca_path
  • ssl_ca_data
  • ssl_password
  • ssl_validate_ocsp
  • ssl_validate_ocsp_stapled
  • ssl_ocsp_context
  • ssl_ocsp_expected_cert
  • redis_connect_func
  • credential_provider

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

Thanks! This becomes a bit repetitive, maybe we should use Unpack[] with a TypedDict (PEP-692) to define these kwargs?

@jonathan-h-grebe
Copy link
Author

Thanks! This becomes a bit repetitive, maybe we should use Unpack[] with a TypedDict (PEP-692) to define these kwargs?

Good point - those (almost) identical parameters being repeated 5 times do seem a bit much.

However, as TypedDicts don't support default values for keys, it wouldn't be possible to define the parameter defaults.
We could get around that by defining a dataclass, but I think that would quite an impact on existing code bases, having to switch from dict to class based syntax.

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.

None yet

2 participants