Add: Services to system #883
Conversation
Misha-Shvets
force-pushed
the
services_to_system_1084
branch
from
38592fb to
88cc71a
Compare
milov-dmitriy
left a comment
milov-dmitriy
left a comment
There was a problem hiding this comment.
главное проверь это локально любым из способов, главное чтобы переименование сработало корректно и даунгрейд тоже сработал корректно
app/ldap_protocol/kerberos/utils.py
Outdated
| def get_services_container_dn(base_dn: str) -> str: | ||
| """Get System container DN for services.""" | ||
| return f"ou=System,{base_dn}" |
There was a problem hiding this comment.
а это точно к керберосу относится а не к лдапу?
There was a problem hiding this comment.
и еще надо будет подлить к тебе мою задачу 796, т.к. я в ней добавляю понятие системная директория или нет, если системная - значит ее нельзя менять
upd. но это уже после того как ее апрувнут, но я бы предлагал сначала мою апрувать потом твою
UPD. Всё ок
| await session.flush() | ||
| await _update_descendants(session, services_dir.id) | ||
|
|
||
| await _update_attributes(session, "ou=services", "ou=System") |
There was a problem hiding this comment.
предлагаю строго привязывать к id той директории которую мы обновили, т.е. передавать в _update_attributes еще и directory_id, чтобы не сделать "лишних" изменений у тех директорий, которые в отбор не попали, но атрибуты которых попали под фильтр атрибутов. думаю, это повысит надежность миграции
There was a problem hiding this comment.
там вроде все надежно, везде где встретим надо менять
There was a problem hiding this comment.
я бы лучше сделал через явное указание id тех директорий, атрибуты которых надо поменять
|
upd. надо будет еще свежий dev подлить UPD. Всё ок |
…or services and system
…es for Kerberos configuration files
…es, enhance config update script, and simplify test case
…e related paths and attributes recursively. Implement downgrade functionality to revert changes.
… the renaming process of 'services' to 'System', including updates to paths and attributes. Enhance downgrade functionality for reverting changes.
Misha-Shvets
force-pushed
the
services_to_system_1084
branch
from
98768c3 to
60ae9bd
Compare
…tion sequence for renaming 'services' to 'System'.
… across the codebase to align with updated terminology and improve clarity in Kerberos-related functionality.
There was a problem hiding this comment.
Pull request overview
This PR renames the LDAP container from ou=services to ou=System for Active Directory compatibility. The change updates configuration files, database migrations, tests, and introduces a volume-based approach for updating Kerberos configurations instead of using the API.
Changes:
- Renamed
ou=servicestoou=Systemacross codebase and tests - Added database migration to rename existing services containers and update all dependent paths/attributes
- Refactored Kerberos config updates to use direct file writes via shared volume instead of API calls
- Updated test assertions to reflect new naming convention
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_shedule.py | Removed kadmin parameter from test function |
| tests/test_api/test_main/test_kadmin.py | Updated test assertions to use System instead of services |
| interface | Updated subproject commit reference |
| docker-compose.yml | Added kdc volume mount for scheduler service |
| app/ldap_protocol/roles/role_use_case.py | Updated to use get_system_container_dn helper function |
| app/ldap_protocol/kerberos/utils.py | Added get_system_container_dn utility function |
| app/ldap_protocol/kerberos/service.py | Refactored to use get_system_container_dn helper |
| app/extra/scripts/update_krb5_config.py | Completely refactored to write configs directly to files with legacy DN migration |
| app/alembic/versions/a1b2c3d4e5f6_rename_services_to_system.py | Added migration to rename services to System with upgrade/downgrade support |
| .kerberos/entrypoint.sh | Added blank line for formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| await session.flush() | ||
|
|
||
| async def _rename_system_to_services(connection: AsyncConnection) -> None: # noqa ARG001 |
There was a problem hiding this comment.
Missing colon after 'noqa ARG001' comment directive.
| async def _rename_system_to_services(connection: AsyncConnection) -> None: # noqa ARG001 | |
| async def _rename_system_to_services(connection: AsyncConnection) -> None: # noqa: ARG001 |
| - ./certs:/certs | ||
| - ./app:/app | ||
| - ldap_keytab:/LDAP_keytab/ | ||
| - kdc:/etc/krb5kdc/ |
There was a problem hiding this comment.
в другие композы продублировать, особенно в .package, там продовый валяется
| def downgrade(container: AsyncContainer) -> None: | ||
| """Downgrade: Rename 'System' container back to 'services'.""" | ||
|
|
||
| async def _update_descendants_downgrade( |
There was a problem hiding this comment.
а функции эти не одинаковые с upgrade? очень прям похожие, лучше вынеси
Переименование раздела
servicesвSystemпо аналогии с АДнюансы: Пока сделал обновление конфигов кербероса не по апи, а через volume, т.к. когда мы меняем схему, то kerberos валится при fetch данных и мы не можем дойти с запросом на обновление конфига
Задача: 1084