-
Notifications
You must be signed in to change notification settings - Fork 164
feat(BA-4178): Add scope types - resource group, container registry, and artifact registry #8491
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
base: main
Are you sure you want to change the base?
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 implements scope search functionality for three new scope types (resource group, container registry, and artifact registry) and adds a fourth scope type (storage host) that raises NotImplementedError since its data resides in etcd.
Changes:
- Added RESOURCE_GROUP, CONTAINER_REGISTRY, ARTIFACT_REGISTRY, and STORAGE_HOST to the ScopeType enum
- Implemented complete scope search infrastructure (conditions, orders, queriers, and database methods) for resource group, container registry, and artifact registry
- Added NotSupportedOrderingType error for scope types that don't support CREATED_AT ordering (container and artifact registries)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/common/data/permission/types.py | Added four new scope types to ScopeType enum |
| src/ai/backend/manager/repositories/permission_controller/repository.py | Added case statements to route new scope types to appropriate db_source methods |
| src/ai/backend/manager/repositories/permission_controller/options.py | Implemented condition and order classes for filtering and sorting the three new database-backed scope types |
| src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py | Added search methods for querying resource groups, container registries, artifact registries, and storage hosts (raises NotImplementedError) |
| src/ai/backend/manager/errors/permission.py | Added NotSupportedOrderingType error for handling unsupported ordering operations |
| src/ai/backend/manager/api/rbac/scope_adapter.py | Implemented adapter methods to build queriers and convert filters/orders for the new scope types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def search_resource_group_scopes( | ||
| self, | ||
| querier: BatchQuerier, | ||
| ) -> ScopeListResult: | ||
| async with self._db.begin_readonly_session_read_committed() as db_sess: | ||
| query = sa.select(ScalingGroupRow.name) | ||
| result = await execute_batch_querier( | ||
| db_sess, | ||
| query, | ||
| querier, | ||
| ) | ||
|
|
||
| items = [ | ||
| ScopeData( | ||
| id=ScopeId(scope_type=ScopeType.RESOURCE_GROUP, scope_id=row.name), | ||
| name=row.name, | ||
| ) | ||
| for row in result.rows | ||
| ] | ||
|
|
||
| return ScopeListResult( | ||
| items=items, | ||
| total_count=result.total_count, | ||
| has_next_page=result.has_next_page, | ||
| has_previous_page=result.has_previous_page, | ||
| ) |
Copilot
AI
Jan 30, 2026
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.
The new scope search methods for RESOURCE_GROUP, CONTAINER_REGISTRY, and ARTIFACT_REGISTRY lack test coverage. The existing test file tests/unit/manager/repositories/permission_controller/test_search_scopes.py has comprehensive tests for DOMAIN, PROJECT, USER, and GLOBAL scope types, but does not include tests for the newly added scope types. Consider adding similar test classes (e.g., TestSearchResourceGroupScopes, TestSearchContainerRegistryScopes, TestSearchArtifactRegistryScopes) to ensure the new functionality works correctly.
src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py
Outdated
Show resolved
Hide resolved
src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py
Outdated
Show resolved
Hide resolved
src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py
Outdated
Show resolved
Hide resolved
3a5f8e0 to
701f98b
Compare
| USER = "user" | ||
| GLOBAL = "global" | ||
|
|
||
| # Scopes that are also treated as entity types |
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 a question.
Could domain, user, project also be treated as entity types?
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.
yes, they are. this comment is confusing so I deleted
| async def search_storage_host_scopes( | ||
| self, | ||
| querier: BatchQuerier, | ||
| ) -> ScopeListResult: | ||
| raise NotImplementedError("Storage host data is stored in etcd, not in the database.") |
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 we'd just better remove this (it should be etcd_source or other)
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.
since we have a plan to migrate the data from etcd to db, I left a TODO comment here
701f98b to
6d86ff7
Compare
| ) | ||
|
|
||
|
|
||
| class NotSupportedOrderingType(BackendAIError, web.HTTPBadRequest): |
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 does seem close to UnsupportedOrderingType, but this error doesn't appear necessary. (I believe the type checker should catch it.)
| querier: BatchQuerier, | ||
| ) -> ScopeListResult: | ||
| async with self._db.begin_readonly_session_read_committed() as db_sess: | ||
| query = sa.select(ScalingGroupRow.name) |
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.
Do we really have to query just the Name?
| case ScopeType.RESOURCE_GROUP: | ||
| return await self._db_source.search_resource_group_scopes(querier) | ||
| case ScopeType.CONTAINER_REGISTRY: | ||
| return await self._db_source.search_container_registry_scopes(querier) | ||
| case ScopeType.ARTIFACT_REGISTRY: | ||
| return await self._db_source.search_artifact_registry_scopes(querier) | ||
| case ScopeType.STORAGE_HOST: | ||
| return await self._db_source.search_storage_host_scopes(querier) |
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 don't understand why this is necessary. We agreed not to include repository queries like this in the internal implementation. This implementation should not be included.
6d86ff7 to
8f966e8
Compare
resolves #8486 (BA-4178)
Summary
Checklist: (if applicable)
ai.backend.testdocsdirectory