Skip to content

Conversation

@daric93
Copy link
Owner

@daric93 daric93 commented Dec 8, 2025

No description provided.

@daric93 daric93 self-assigned this Dec 8, 2025
"""
return int(super().get_exposed_port(self.port))

@wait_container_is_ready(ValkeyNotReady)

Choose a reason for hiding this comment

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

Deprecated Decorator Usage

Location: modules/valkey/testcontainers/valkey/__init__.py, line 113

Current Code:

@wait_container_is_ready(ValkeyNotReady)
def _connect(self) -> None:
    """Wait for Valkey to be ready by sending PING command."""
    # ... connection logic

Issue:

The @wait_container_is_ready decorator is deprecated in the codebase. Recent commits show active migration away from this pattern:

Evidence from Codebase:

# From pytest.ini_options filterwarnings:
"ignore:The @wait_container_is_ready decorator is deprecated.*:DeprecationWarning"

Recommended Fix:

Migrate to modern wait strategy pattern:

from testcontainers.core.wait_strategies import ExecWaitStrategy

class ValkeyContainer(DockerContainer):
    def __init__(self, image: str = "valkey/valkey:latest", port: int = 6379, **kwargs) -> None:
        super().__init__(image, **kwargs)
        self.port = port
        self.password: Optional[str] = None
        self.with_exposed_ports(self.port)
        
    def start(self) -> "ValkeyContainer":
        # Build wait strategy based on password
        if self.password:
            # Use custom wait strategy for authenticated connections
            self.waiting_for(self._create_auth_wait_strategy())
        else:
            # Use exec strategy for simple PING
            self.waiting_for(
                ExecWaitStrategy(["valkey-cli", "ping"])
            )
        super().start()
        return self

Benefits:

  • Aligns with project direction
  • More composable and testable
  • Consistent with other modules
  • Better error messages

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed

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.

3 participants