chore: sync with upstream API changes 2026-06-12#29
Conversation
There was a problem hiding this comment.
Two issues found, both around test coverage. Backwards compatibility is clean — all changes are additive.
🟠 Missing client tests: get_orphan_report and confirm_orphans are not tested in test_client.py. Every other admin client method has a dedicated test (see test_list_admin_marked_* and test_delete_admin_mark_*). These should cover the HTTP path, payload shape, and basic response handling.
🟠 Missing CLI integration tests: test_cli.py has tests for admin delete dry-run and help output, but orphan-report and orphan-confirm have no CLI-level tests. Missing cases: orphan-report basic invocation, orphan-confirm --dry-run showing the endpoint, orphan-confirm with no --item (should exit 1), and orphan-confirm with an invalid item format.
🟡 keycloak_client realm not validated client-side (see inline): The API docs say realm is required for keycloak_client, but the CLI will silently send a payload without it and let the server return an error. A client-side check would give a clearer error message.
No backwards-incompatible changes. Design principles are followed: read-only orphan-report has no --yes/--dry-run, mutating orphan-confirm has both plus @handle_api_errors, dry-run is checked before confirm_action.
e4bbe87 to
2e148ad
Compare
There was a problem hiding this comment.
All three findings from the previous review have been addressed — client tests, CLI integration tests, and the keycloak_client realm validation are all now present.
Three minor issues remain (see inline comments):
🟡 Test docstring (test_client.py): says "v1 GET" — should be "v2 GET".
🟡 Empty name not rejected (admin.py): --item "postgresql_database:" yields name="" and passes client-side validation. Add if not name: after the split.
🟡 Empty realm not rejected (admin.py): --item "keycloak_client:name:" sets entry["realm"] = "", so "realm" not in entry is False and the guard is bypassed. Change to if not entry.get("realm"):.
No backwards-incompatible changes. Design principles are followed throughout: orphan-report is correctly read-only (no --yes/--dry-run), orphan-confirm has both plus @handle_api_errors, dry-run is checked before confirm_action, and _request (not _async_request) is correctly used since both endpoints return 200, not 202.
|
Review-opmerkingen verwerkt:
Branch is gerebased op main. Alle tests groen, lint en format schoon. |
Implements two new upstream endpoints: - GET /api/v2/admin/orphans/report → `zad admin orphan-report` - POST /api/v2/admin/orphans/confirm → `zad admin orphan-confirm` The report command is read-only and shows the classified orphan sweep. The confirm command marks selected items for grace-period deletion via repeatable --item TYPE:NAME (or TYPE:NAME:REALM for keycloak_client).
- client-tests voor get_orphan_report en confirm_orphans - CLI-tests voor orphan-report/orphan-confirm (dry-run, ontbrekend item, ongeldig formaat, onbekend type, keycloak_client zonder realm) - valideer item-type client-side en eis een realm voor keycloak_client
2e148ad to
b8b4ec4
Compare
There was a problem hiding this comment.
Two minor findings; no critical or significant issues.
What's correct:
- Both commands are purely additive — backwards-compat baseline updated correctly
orphan-confirmfollows all mutating-command conventions:@handle_api_errors,--dry-runbeforeconfirm_action,--yes/-y,render_success- Client methods use
_request(sync), which is correct: the OpenAPI spec shows 200 responses, not 202, so_async_requestwould be wrong here - No project requirement on admin commands — correct, these are cluster-level ops
- Input validation is client-side before any API call — good defensive design
ORPHAN_TYPES/ORPHAN_TYPES_REQUIRING_REALMconstants make the validation easy to extend
| result = client.delete_admin_mark(mark_id) | ||
| formatter.render(result) | ||
| formatter.render_success(f"Deletion mark '{mark_id}' removed.") | ||
|
|
There was a problem hiding this comment.
🟡 Minor — non-standard verb vocabulary
CLAUDE.md defines a binding verb table: list, create, add, delete, describe, status, refresh, update-image, check, assign. The sub-command names here use report and confirm, neither of which appears in that table.
orphan-report is the closer call — orphan-status would map to "show current state" semantics, though an active sweep is arguably more than a status read. orphan-confirm has no clean equivalent in the vocabulary.
This may be an intentional exception for admin-specific semantics with no better fit, but it deviates from the stated binding rules. If there is a deliberate exception policy for admin commands, it would be worth documenting in CLAUDE.md.
| assert "orphan-report" in out | ||
| assert "orphan-confirm" in out | ||
|
|
||
|
|
There was a problem hiding this comment.
🟡 Minor — no happy-path CLI test for orphan-report
orphan-report is read-only with no --dry-run flag, so the only meaningful CLI-level test path is a real execution with a mocked HTTP response. The current tests only verify help-text content (--dry-run and --yes are absent), which does not exercise the render path.
The client-level test (test_get_orphan_report_returns_json) covers the API call, but a subprocess test that mocks GET /v2/admin/orphans/report and checks formatter.render() output would catch regressions in the command plumbing. Compare test_confirm_orphans_sends_items_payload — a parallel test using respx on the CLI level would be the right companion here.
API Sync - 2026-06-12
Upstream API changes detected and auto-implemented by Claude.
API diff
New Endpoints: 2
POST /api/v2/admin/orphans/confirm
GET /api/v2/admin/orphans/report
Deleted Endpoints: None
Modified Endpoints: None
Coverage
Upstream API: 46 current endpoints
Covered by CLI: 36
Not covered: 10
Deprecated v1: 10 (skipped, CLI uses v2)
Non-API/infra: 0 (skipped)
Uncovered endpoints:
GET /api/federation/health
Federation Health [federation]
GET /api/federation/peers
List Peers [federation]
POST /api/projects/{project_name}/registries/by-credentials
Add Registry By Credentials [v1 (deprecated)]
POST /api/projects/{project_name}/registries/by-secret
Add Registry By Secret [v1 (deprecated)]
POST /api/tasks
Create Task [tasks]
POST /api/v1/projects/{project_name}/images/push
Push Image [images]
POST /api/v2/admin/cleanup/trigger
Trigger Cleanup [admin]
POST /api/v2/admin/orphans/confirm
Confirm Orphans [admin]
GET /api/v2/admin/orphans/report
Orphan Sweep Report [admin]
POST /api/v2/admin/reconciliation/trigger
Trigger Reconciliation [admin]
Review carefully before merging. All changes should be additive only.