- 
                Notifications
    You must be signed in to change notification settings 
- Fork 31
fix: ensure max_connection accepts default/None #515
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
Conversation
3aba909    to
    834909a      
    Compare
  
    | REDIS_DB: int = 15 | ||
|  | ||
| REDIS_MAX_CONNECTIONS: int = 10 | ||
| REDIS_MAX_CONNECTIONS: Optional[int] = 10 | 
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 should default to None, unless a user specifically wants to block their Redis connections.
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.
Updated to default None.
| REDIS_DB: int = 15 | ||
|  | ||
| REDIS_MAX_CONNECTIONS: int = 10 | ||
| REDIS_MAX_CONNECTIONS: Optional[int] = 10 | 
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 should default to None, unless a user specifically wants to block their Redis connections.
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.
Updated to default None.
|  | ||
| REDIS_MAX_CONNECTIONS: int = 10 | ||
| REDIS_MAX_CONNECTIONS: Optional[int] = 10 | ||
| REDIS_RETRY_TIMEOUT: bool = True | 
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.
BTW. you could extract the common fields from these two Pydantic classes into a parent class that both inherit from
| if v < 1: | ||
| raise ValueError("REDIS_MAX_CONNECTIONS must be at least 1") | ||
| return v | ||
| def validate_max_connections_sentinel(cls, v: Any) -> Optional[int]: | 
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.
We don't need that validator anymore. Default value is None and it accepts int or None as the type.
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.
Removed the validation, although it was adding additional validation if the user tried to add max_connection as null, None, or any other incompatible value in the env vars.
|  | ||
| @field_validator("REDIS_HEALTH_CHECK_INTERVAL") | ||
| @classmethod | ||
| def validate_health_check_interval_sentinel(cls, v: int) -> int: | 
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.
We don't need these validators or the one below. you can just use Field and set a constraint that it should be greater than 0
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.
You need to add this field as a Field and set it so it's greater than zero
https://docs.pydantic.dev/latest/concepts/fields/
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 have replaced the @field_validator with Field to validate the REDIS_HEALTH_CHECK_INTERVAL env var. Moving forward, this much be int  greater than 0.
53ab6b0    to
    abfc3be      
    Compare
  
    abfc3be    to
    b477239      
    Compare
  
    **Related Issue(s):** - None **Description:** - Ensure `REDIS_MAX_CONNECTION` can accept `None` and integer value for default number of connection. [#515](#515) **PR Checklist:** - [x] Code is formatted and linted (run `pre-commit run --all-files`) - [x] Tests pass (run `make test`) - [x] Documentation has been updated to reflect changes, if applicable - [x] Changes are added to the changelog
Description:
This PR updates the handling of the
max_connectionparameter so that it correctly acceptsNoneas the default or integer values. Previously, only integer values were allowed, which caused errors when the parameter was unset or set as None.PR Checklist:
pre-commit run --all-files)make test)