Fix DockerRegistryCredentials and DockerHost to fallback to standard logger#124
Conversation
…logger - Update DockerRegistryCredentials.login() to catch MissingContextError and fallback to a standard prefect logger when not in a flow run context - Update DockerHost.get_client() with the same fallback pattern - Update DockerRegistryCredentials code example to show loading block by name to match the default behavior of other blocks Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
…yCredentials - Add test_get_client_works_outside_flow_run_context for DockerHost - Add test_docker_registry_credentials_login_works_outside_flow_run_context for DockerRegistryCredentials - Both tests verify the logger fallback works without raising MissingContextError Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
Code Review by Qodo (Alpha)
1. `logger` undefined in `get_client`
|
| try: | ||
| logger = get_run_logger() | ||
| except MissingContextError: | ||
| pass |
There was a problem hiding this comment.
1. logger undefined in get_client 📘 Rule Violation
• DockerHost.get_client() catches MissingContextError but does not initialize a fallback logger, leaving logger potentially undefined. • When the exception path is taken, subsequent logger.debug(...) calls will raise UnboundLocalError, preventing client creation and breaking usage outside a flow/task run context. • This is an unhandled edge case introduced by the change, reducing graceful degradation and debuggability.
Agent Prompt
## Issue description
`DockerHost.get_client()` can leave `logger` undefined when `get_run_logger()` raises `MissingContextError`, but the method still calls `logger.debug(...)` later. This can raise `UnboundLocalError` and break creating a Docker client outside a run context.
## Issue Context
The change introduced an exception handler that does `pass` instead of falling back to a standard logger (similar to `DockerRegistryCredentials.login`). The method must gracefully degrade when there is no run context.
## Fix Focus Areas
- src/integrations/prefect-docker/prefect_docker/host.py[94-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| credentials = DockerRegistryCredentials( | ||
| username="test_user", | ||
| password="test_password", | ||
| registry_url="registry.hub.docker.com", | ||
| ) | ||
| # This should not raise MissingContextError | ||
| await credentials.login(client=mock_docker_client) | ||
| mock_docker_client.login.assert_called_once_with( | ||
| username="test_user", | ||
| password="test_password", | ||
| registry="registry.hub.docker.com", | ||
| ) |
There was a problem hiding this comment.
2. Test misses reauth arg 🐞 Bug
• DockerRegistryCredentials.login always passes reauth=self.reauth to client.login. • The new test asserts mock_docker_client.login.assert_called_once_with(...) without including reauth, so it will fail due to argument mismatch.
Agent Prompt
### Issue description
The new unit test asserts `client.login` was called without the `reauth` kwarg, but the implementation always passes `reauth=self.reauth`. This makes the new test fail.
### Issue Context
`DockerRegistryCredentials.reauth` defaults to `True` and is always forwarded to `client.login`.
### Fix Focus Areas
- src/integrations/prefect-docker/tests/test_credentials.py[21-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Benchmark PR from qodo-benchmark#391