refactor(BA-2969, BA-2970): Introduce DB Source in container registry repository #6671
+434
−448
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
size:XL
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the container registry repository layer to use a source-based architecture, extracting database operations into a separate ContainerRegistryDBSource class and converting tests from mocked unit tests to integration tests using a real database.
Key Changes:
- Extracted database access logic into a new
ContainerRegistryDBSourceclass for better separation of concerns - Converted all tests from mocked unit tests to integration tests that use real database operations
- Consolidated admin repository tests into the main test file (though test coverage was lost in the process)
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/repositories/container_registry/repository.py |
Refactored to delegate all database operations to ContainerRegistryDBSource instead of containing DB logic directly |
src/ai/backend/manager/repositories/container_registry/admin_repository.py |
Updated to use ContainerRegistryDBSource directly instead of wrapping ContainerRegistryRepository |
src/ai/backend/manager/repositories/container_registry/db_source/db_source.py |
New file containing all database access logic extracted from the repository classes |
src/ai/backend/manager/repositories/container_registry/db_source/__init__.py |
New empty __init__.py file for the db_source package |
tests/manager/repositories/container_registry/test_container_registry_repository.py |
Converted from mocked unit tests to integration tests with real database operations and helper fixtures |
tests/manager/repositories/container_registry/test_admin_container_registry_repository.py |
Deleted - admin repository tests were intended to be consolidated but coverage was lost |
tests/manager/repositories/container_registry/BUILD |
Added explicit dependency on the manager source code |
changes/6671.enhance.md |
Added changelog entry describing the refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/manager/repositories/container_registry/test_container_registry_repository.py
Show resolved
Hide resolved
tests/manager/repositories/container_registry/test_container_registry_repository.py
Show resolved
Hide resolved
src/ai/backend/manager/repositories/container_registry/db_source/db_source.py
Show resolved
Hide resolved
src/ai/backend/manager/repositories/container_registry/db_source/db_source.py
Show resolved
Hide resolved
HyeockJinKim
requested changes
Nov 10, 2025
Comment on lines
+96
to
+100
| async def fetch_registry_row( | ||
| self, | ||
| registry_name: str, | ||
| project: Optional[str] = None, | ||
| ) -> ContainerRegistryRow: |
Collaborator
There was a problem hiding this comment.
Depending on how the unique condition of the registry name is applied in this method, it seems like multiple values could be queried, so I need to check the behavior.
Comment on lines
+81
to
+86
| async def fetch_known_registries(self) -> dict[str, str]: | ||
| """Fetch all known container registries from the database.""" | ||
| async with self._db.begin_readonly_session() as session: | ||
| known_registries_map = await ContainerRegistryRow.get_known_container_registries( | ||
| session | ||
| ) |
Collaborator
There was a problem hiding this comment.
Please avoid returning a dict. You should properly define and pass the data class from where you first read the value, so that subsequent logic becomes easier to read.
Comment on lines
+72
to
+77
| .where(ImageRow.registry == registry_name) | ||
| .where(ImageRow.status != ImageStatus.DELETED) | ||
| .values(status=ImageStatus.DELETED) | ||
| ) | ||
| if project: | ||
| update_stmt = update_stmt.where(ImageRow.project == project) |
Collaborator
There was a problem hiding this comment.
We also need to check the project and registry parts once.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #6643, #6644 (BA-2969, BA-2970)
Checklist: (if applicable)
ai.backend.testdocsdirectory