-
Notifications
You must be signed in to change notification settings - Fork 164
feat(BA-1843): Separate repository layer from container registry service #5095
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
feat(BA-1843): Separate repository layer from container registry service #5095
Conversation
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.
Pull Request Overview
This PR separates the container registry persistence logic into its own repository layer and updates the service and tests to use that new layer instead of working directly with the database engine.
- Introduce
ContainerRegistryRepository
andAdminContainerRegistryRepository
for read and write operations. - Refactor
ContainerRegistryService
and the service factory to accept and use these repositories. - Update test fixtures to provide a
repositories
fixture and wire it into container registry tests.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/manager/services/container_registry/conftest.py | Updated processors fixture to use the new repositories fixture. |
tests/manager/conftest.py | Added async repositories fixture and imports for the new layer. |
src/ai/backend/manager/services/processors.py | Inject container registry repositories into the service factory. |
src/ai/backend/manager/services/container_registry/service.py | Refactored service methods to use the new repository layer. |
src/ai/backend/manager/repositories/repositories.py | Registered ContainerRegistryRepositories in the central factory. |
src/ai/backend/manager/repositories/container_registry/repository.py | Implemented read-only repository for container registry data. |
src/ai/backend/manager/repositories/container_registry/admin_repository.py | Implemented admin repository for clearing images. |
src/ai/backend/manager/repositories/container_registry/repositories.py | Defined repository aggregate and RepositoryArgs dataclasses. |
src/ai/backend/manager/repositories/container_registry/BUILD | Added BUILD file for the new repository package. |
Comments suppressed due to low confidence (2)
src/ai/backend/manager/repositories/container_registry/repository.py:1
- The new repository class introduces several methods but there are no direct unit tests for it. Consider adding tests for
get_registry_by_name_and_project
,get_all_registries
, andget_known_registries
to ensure correct behavior.
from typing import Optional
tests/manager/conftest.py:1091
- This async fixture may require the pytest-asyncio plugin. If you’re using pytest-asyncio, annotate this with
@pytest_asyncio.fixture
to ensure the event loop is available.
@pytest.fixture
result = await scanner.rescan_single_registry(action.progress_reporter) | ||
registry_row = ContainerRegistryRow.from_dataclass(registry_data) | ||
scanner_cls = get_container_registry_cls(registry_row) | ||
scanner = scanner_cls(self._repository._db, registry_name, registry_row) |
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.
Accessing the private _db
attribute on the repository ties the service to implementation details. Consider exposing a session or engine getter on the repository or passing the engine explicitly to the service to avoid using a private property.
scanner = scanner_cls(self._repository._db, registry_name, registry_row) | |
scanner = scanner_cls(self._repository.get_db_session(), registry_name, registry_row) |
Copilot uses AI. Check for mistakes.
3b9dc9b
to
bf410c6
Compare
from .repository import ContainerRegistryRepository | ||
|
||
|
||
class AdminContainerRegistryRepository: |
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.
It would be good to add a permission check hook that runs before the method is executed.
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 action will all be removed when RBAC is applied, so for now, it is being left as is.
@@ -0,0 +1 @@ | |||
Separate repository layer from container registry service |
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.
Separate repository layer from container registry service | |
Separate admin repository layer from container registry service |
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 think the current message is appropriate because the important thing is not separating the admin, but separating the repository layer from the service.
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.
Isn't the container repository already separated, and isn't this PR about separating the admin container repository layer?
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 didn't consider it separated because, although there was a repository class, the actual implementation wasn't separated.
resolves #5093 (BA-1843)
Checklist: (if applicable)
ai.backend.test
docs
directory