Skip to content

fix(BA-6922): pluralize the app config allow-list REST v2 prefix#13034

Closed
jopemachine wants to merge 2 commits into
mainfrom
fix/app-config-rest-v2-conventions
Closed

fix(BA-6922): pluralize the app config allow-list REST v2 prefix#13034
jopemachine wants to merge 2 commits into
mainfrom
fix/app-config-rest-v2-conventions

Conversation

@jopemachine

Copy link
Copy Markdown
Member

Related: #12929 (BA-6922)

Summary

Two REST v2 convention gaps in the app config family, found while reviewing the fragment search surface.

1. app-config-allow-list was the only singular entity prefix. Every other v2 entity collection is plural:

prefix
before /v2/app-config-allow-list
after /v2/app-config-allow-lists
siblings app-config-definitions, app-config-fragments, resource-policies, model-cards, runtime-variants, …

Renamed across the route registry, the SDK _PATH, its tests, and openapi.json. The CLI command name stays singular (bai admin app-config-allow-list) — every v2 admin group is singular there (resource-policy, model-card, app-config-definition), so the CLI was already right.

2. The missing scoped search on allow-list and definition is now stated, not implied. api/rest/v2/AGENTS.md says a search always comes in two variants — a superadmin /search and a scoped one — and both registries only have the first. That is correct here: both are admin-only entities (every route is superadmin_required) and a non-admin reads them through the app config resolve path rather than querying them. The registry docstrings now say so, so the omission does not read as an oversight to the next person checking the convention.

Breaking change

/v2/app-config-allow-list/*/v2/app-config-allow-lists/*. The bundled SDK is updated in the same commit, so bai and the Python client keep working; any caller hitting the raw path needs the new one.

Not in this PR

POST /v2/app-config-fragments/scoped-search does not match either documented form — the current /{entity}/{scope_type}/{scope_id}/search or the forward /{entity}/scoped/search (cf. scheduling-history's /kernels/scoped/search). That route is added by #13020 and does not exist on main, so it cannot be fixed from a main-based branch; it is corrected on that PR instead.

Test plan

  • Formatter, linter, visibility and mypy clean over the touched manager, client and test packages
  • openapi.json regenerated with ./backend.ai mgr api dump-openapi; the delta is exactly the three allow-list paths
  • Verified no other reference to the singular path remains (errors/app_config.py keeps probs/app-config-allow-list-not-found, which is an error-type URN, not a route)
  • CI verification

🤖 Generated with Claude Code

Every REST v2 entity collection is registered under a plural prefix —
`app-config-definitions`, `app-config-fragments`, `resource-policies`,
`model-cards` — except `app-config-allow-list`, which was singular. Rename it to
`app-config-allow-lists` across the route registry, the SDK path and its tests.

The CLI command name stays singular (`bai admin app-config-allow-list`), matching
every other v2 admin group: `resource-policy`, `model-card`,
`app-config-definition`.

Also record why the allow-list and definition registries have no scoped search
counterpart to `/search`. The convention pairs a superadmin `/search` with a
scoped variant, and both omit it — deliberately, since they are admin-only
entities that a non-admin reads through the app config resolve path rather than
querying directly. Saying so keeps the omission from reading as an oversight.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 07:57
@jopemachine
jopemachine requested a review from a team as a code owner July 22, 2026 07:57
@github-actions github-actions Bot added size:M 30~100 LoC area:docs Documentations comp:manager Related to Manager component comp:client Related to Client component labels Jul 22, 2026
@jopemachine
jopemachine marked this pull request as draft July 22, 2026 07:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Pluralizes the REST v2 app-config allow-list collection prefix and updates consumers and documentation accordingly.

Changes:

  • Renames the route to /v2/app-config-allow-lists.
  • Updates the SDK, tests, OpenAPI specification, and changelog.
  • Documents why scoped search is omitted for admin-only entities.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/unit/client_v2/test_app_config_allow_list.py Updates SDK path assertions.
src/ai/backend/manager/api/rest/v2/app_config_definition/registry.py Documents scoped-search omission.
src/ai/backend/manager/api/rest/v2/app_config_allow_list/registry.py Renames the route prefix and adds rationale.
src/ai/backend/client/v2/domains_v2/app_config_allow_list.py Updates the SDK endpoint prefix.
docs/manager/rest-reference/openapi.json Regenerates affected OpenAPI paths.
changes/13034.breaking.md Records the breaking route rename.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +22 to +24
The allow-list is an admin-only entity — every route is ``superadmin_required`` — so
it carries no scoped search counterpart to ``/search``. A non-admin never queries the
allow-list directly; it is read through the app config resolve path.
Comment on lines +22 to +24
A config definition is an admin-only entity — every route is ``superadmin_required`` —
so it carries no scoped search counterpart to ``/search``. A non-admin never queries
definitions directly; they are read through the app config resolve path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:docs Documentations comp:client Related to Client component comp:manager Related to Manager component size:M 30~100 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants