Skip to content

Commit 247924f

Browse files
committed
refactor(server): introduce services layer with typed Server model
First step of the incremental migration spelled out in ADR-0007 (which supersedes ADR-0004's "no services layer" stance). Sets up the two new packages and converts the four most-used server subcommands as a proof of concept. New layout: - orca_cli/models/server.py — TypedDict view of the Nova server payload covering the fields orca actually reads (standard names + the OS-EXT-* / OS-SRV-USG / os-extended-volumes colon-prefixed keys). - orca_cli/services/server.py — ServerService wraps OrcaClient and owns /servers URL construction. Methods: find/find_all (paginated read), get, delete, action, start, stop, reboot. The list operation is named find/find_all so list[Server] annotations on sibling methods resolve to the builtin instead of the method itself. Migrated commands: - server list, server show, server delete, server reboot The remaining 30+ subcommands continue to call OrcaClient directly until they're each migrated in their own commit; ADR-0007 tracks progress. Drive-by: - print_list signature widened from list[dict] to list (any sequence of mapping-like items, so TypedDict instances pass). - server_show's local fields annotation widened from list[tuple[str, str]] to list[tuple[str, Any]] — the second element is whatever the Nova field returns, stringified by print_detail. - ADR-0004 marked Superseded by ADR-0007; mkdocs nav and the ADR index updated.
1 parent 264798e commit 247924f

10 files changed

Lines changed: 240 additions & 22 deletions

File tree

docs/adr/0004-no-services-layer.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# ADR-0004: No services layer (yet)
22

3-
**Status**: Accepted
3+
**Status**: Superseded by [ADR-0007](0007-incremental-services-layer.md)
44
**Date**: 2026-04-20
55

66
## Context
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
# ADR-0007: Incremental services layer with typed models
2+
3+
**Status**: Accepted
4+
**Date**: 2026-04-20
5+
**Supersedes**: [ADR-0004](0004-no-services-layer.md)
6+
7+
## Context
8+
9+
ADR-0004 deferred the services layer because the cost of refactoring all
10+
63 command modules at once was disproportionate to the immediate gain.
11+
Two months later the friction it predicted has materialised:
12+
13+
- 740-odd raw `client.get/post/...` calls scattered across commands made
14+
the recent N+1 hunt (cleanup, watch, export) harder than it should
15+
have been: each fix had to be re-derived from the URL string.
16+
- The `dict[str, Any]` access pattern (2 136 `.get()` calls) means
17+
`srv.get("nmae", "")` typos slip through to runtime; mypy can't help
18+
because everything is `Any`.
19+
- Tests mock at the `OrcaClient` boundary; they assert on the URL string
20+
passed to `client.get`. Renaming an endpoint or changing pagination
21+
scheme breaks the assertions in cosmetic ways.
22+
23+
The original concern — multi-day refactor with high regression risk —
24+
remains valid only if we attempt the migration in one shot.
25+
26+
## Decision
27+
28+
Introduce two new layers, **incrementally, one resource at a time**:
29+
30+
- `orca_cli/models/<resource>.py``TypedDict`s describing the subset
31+
of each Nova/Cinder/Neutron/etc. payload that orca actually reads.
32+
`total=False` everywhere — fields are added as commands need them.
33+
- `orca_cli/services/<resource>.py` — a class wrapping `OrcaClient` that
34+
exposes typed methods (`list`, `get`, `delete`, `reboot`, ...) and
35+
owns the URL construction for that resource.
36+
37+
Migration order is driven by pain, not by alphabetical resource name.
38+
The first migration is `server` (66 raw HTTP calls, 2235 LOC, the most
39+
exercised module). Each migration ships as one commit per command being
40+
refactored, never a "rewrite the whole module" PR. Commands that haven't
41+
been migrated keep calling `OrcaClient` directly — there is no flag day.
42+
43+
The criteria for *not* introducing a service for a given resource:
44+
45+
- Single-call wrapper (`get_token`, `get_catalog`) where the service
46+
would just be a one-liner over the client. Stay direct.
47+
- Resource that orca only reads, never writes, with one or two calls
48+
total. The service overhead exceeds the gain.
49+
50+
## Consequences
51+
52+
- **Positive**: typed return values mean autocomplete, mypy catches
53+
field-name typos, and refactors propagate to compile-time errors.
54+
- **Positive**: URL construction lives in one place per resource; an
55+
OpenStack version bump touches `services/<resource>.py`, not 30
56+
command handlers.
57+
- **Positive**: tests can assert on `service.list.return_value =
58+
[<typed Server>]` rather than mocking `client.get` URL-strings.
59+
Less brittle, more readable.
60+
- **Negative / trade-off**: the codebase has *two* patterns for several
61+
weeks (or months) until every resource is migrated. The boundary is
62+
visible — a command either imports `services.X` or calls `client.get`
63+
directly. We accept the ugliness; a flag-day rewrite was the
64+
alternative we already rejected.
65+
- **Negative / trade-off**: `TypedDict` doesn't enforce field presence
66+
at runtime — `srv["status"]` still raises `KeyError` if missing. The
67+
contract is "*if* this field exists, *then* it has this type". For
68+
presence guarantees we'd need `dataclass` + factory functions; that
69+
conversion can come later if the runtime safety pays off.
70+
71+
## Migration tracking
72+
73+
When migrating a resource, leave a one-line entry below so future ADR
74+
readers can see how far along the work is.
75+
76+
- 2026-04-20 — `server`: ServerService + Server TypedDict, four commands
77+
migrated (`list`, `show`, `delete`, `reboot`). Remaining 30+ server
78+
commands still call `OrcaClient` directly.

docs/adr/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ of the documentation.
3333
- [ADR-0001 — Lazy command registration](0001-lazy-command-registration.md)
3434
- [ADR-0002 — Dual-format configuration file](0002-dual-format-config-file.md)
3535
- [ADR-0003 — Auth-type auto-detection](0003-auth-type-auto-detection.md)
36-
- [ADR-0004 — No services layer (yet)](0004-no-services-layer.md)
36+
- [ADR-0004 — No services layer (yet)](0004-no-services-layer.md) *(superseded by ADR-0007)*
3737
- [ADR-0005 — Durable, atomic token cache](0005-durable-token-cache.md)
3838
- [ADR-0006 — Idempotent-only HTTP retries](0006-idempotent-only-retries.md)
39+
- [ADR-0007 — Incremental services layer with typed models](0007-incremental-services-layer.md)

mkdocs.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,3 +131,4 @@ nav:
131131
- 0004 — No services layer (yet): adr/0004-no-services-layer.md
132132
- 0005 — Durable token cache: adr/0005-durable-token-cache.md
133133
- 0006 — Idempotent-only retries: adr/0006-idempotent-only-retries.md
134+
- 0007 — Incremental services layer: adr/0007-incremental-services-layer.md

orca_cli/commands/server.py

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
from orca_cli.core.output import console, output_options, print_detail, print_list
2323
from orca_cli.core.validators import validate_id
2424
from orca_cli.core.waiter import wait_for_resource
25+
from orca_cli.services.server import ServerService
2526

2627

2728
@click.group()
@@ -39,11 +40,8 @@ def server(ctx: click.Context) -> None:
3940
@click.pass_context
4041
def server_list(ctx: click.Context, limit: int, output_format: str, columns: tuple[str, ...], fit_width: bool, max_width: int | None, noindent: bool) -> None:
4142
"""List servers."""
42-
client = ctx.find_object(OrcaContext).ensure_client()
43-
url = f"{client.compute_url}/servers/detail"
44-
data = client.get(url, params={"limit": limit})
45-
46-
servers = data.get("servers", [])
43+
service = ServerService(ctx.find_object(OrcaContext).ensure_client())
44+
servers = service.find(limit=limit)
4745

4846
def _addresses(srv: dict) -> str:
4947
parts = []
@@ -82,11 +80,8 @@ def _flavor(srv: dict) -> str:
8280
@click.pass_context
8381
def server_show(ctx: click.Context, server_id: str, output_format: str, columns: tuple[str, ...], fit_width: bool, max_width: int | None, noindent: bool) -> None:
8482
"""Show server details."""
85-
client = ctx.find_object(OrcaContext).ensure_client()
86-
url = f"{client.compute_url}/servers/{server_id}"
87-
data = client.get(url)
88-
89-
srv = data.get("server", data)
83+
service = ServerService(ctx.find_object(OrcaContext).ensure_client())
84+
srv = service.get(server_id)
9085

9186
# Power state mapping (Nova integer → label)
9287
power_states = {
@@ -98,7 +93,7 @@ def server_show(ctx: click.Context, server_id: str, output_format: str, columns:
9893
7: "Suspended",
9994
}
10095

101-
fields: list[tuple[str, str]] = []
96+
fields: list[tuple[str, Any]] = []
10297

10398
fields.append(("ID", srv.get("id", "")))
10499
fields.append(("Name", srv.get("name", "")))
@@ -396,27 +391,29 @@ def server_delete(ctx: click.Context, server_id: str, yes: bool, dry_run: bool,
396391
"""Delete a server."""
397392
orca_ctx = ctx.find_object(OrcaContext)
398393
client = orca_ctx.ensure_client()
399-
url = f"{client.compute_url}/servers/{server_id}"
394+
service = ServerService(client)
400395

401396
if dry_run:
402-
data = client.get(url)
403-
srv = data.get("server", data)
397+
srv = service.get(server_id)
404398
console.print("[yellow]Would delete server:[/yellow]")
405399
console.print(f" ID: {srv.get('id', server_id)}")
406400
console.print(f" Name: {srv.get('name', '—')}")
407401
console.print(f" Status: {srv.get('status', '—')}")
408-
console.print(f" Image: {srv.get('image', {}).get('id', '—') if isinstance(srv.get('image'), dict) else '—'}")
402+
image = srv.get("image")
403+
image_id = image.get("id", "—") if isinstance(image, dict) else "—"
404+
console.print(f" Image: {image_id}")
409405
return
410406

411407
if not yes:
412408
click.confirm(f"Delete server {server_id}?", abort=True)
413409

414-
client.delete(url)
410+
service.delete(server_id)
415411
cache.invalidate(orca_ctx.profile, "servers")
416412

417413
if wait:
418414
wait_for_resource(
419-
client, url, "server", "DELETED",
415+
client, f"{client.compute_url}/servers/{server_id}",
416+
"server", "DELETED",
420417
label=f"Server {server_id}",
421418
delete_mode=True,
422419
)
@@ -466,10 +463,11 @@ def server_stop(ctx: click.Context, server_id: str, wait: bool) -> None:
466463
@click.pass_context
467464
def server_reboot(ctx: click.Context, server_id: str, hard: bool, wait: bool) -> None:
468465
"""Reboot a server."""
466+
client = ctx.find_object(OrcaContext).ensure_client()
467+
ServerService(client).reboot(server_id, hard=hard)
469468
reboot_type = "HARD" if hard else "SOFT"
470-
_server_action(ctx, server_id, {"reboot": {"type": reboot_type}}, f"Reboot ({reboot_type})")
469+
console.print(f"[green]Reboot ({reboot_type}) request sent for {server_id}.[/green]")
471470
if wait:
472-
client = ctx.find_object(OrcaContext).ensure_client()
473471
wait_for_resource(client, f"{client.compute_url}/servers/{server_id}",
474472
"server", "ACTIVE", label=f"Server {server_id}")
475473

orca_cli/core/output.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ def _json_indent(noindent: bool) -> int | None:
5757

5858

5959
def print_list(
60-
items: list[dict],
60+
items: list,
6161
column_defs: list[tuple],
6262
*,
6363
title: str = "",

orca_cli/models/__init__.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
"""Typed models for OpenStack API responses (subset used by orca).
2+
3+
Models are TypedDicts with ``total=False`` — the field set we actually
4+
read from each resource, not an exhaustive schema. Adding a field that
5+
mypy starts to flag means widening the type, not silencing the error.
6+
"""

orca_cli/models/server.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
"""Typed view of the Nova server resource (only the fields orca reads)."""
2+
3+
from __future__ import annotations
4+
5+
from typing import TypedDict
6+
7+
8+
class ServerFlavor(TypedDict, total=False):
9+
id: str
10+
original_name: str
11+
vcpus: int
12+
ram: int
13+
disk: int
14+
15+
16+
class ServerAddress(TypedDict, total=False):
17+
addr: str
18+
version: int
19+
20+
21+
# Server uses the alternative TypedDict syntax because Nova exposes a
22+
# handful of OS-EXT-* fields whose names contain colons — those can't
23+
# be expressed as Python identifiers in the class form.
24+
Server = TypedDict(
25+
"Server",
26+
{
27+
"id": str,
28+
"name": str,
29+
"status": str,
30+
"flavor": ServerFlavor,
31+
"image": dict,
32+
"addresses": dict,
33+
"key_name": str,
34+
"created": str,
35+
"updated": str,
36+
"user_id": str,
37+
"tenant_id": str,
38+
"hostId": str,
39+
"accessIPv4": str,
40+
"accessIPv6": str,
41+
"config_drive": str,
42+
"progress": int,
43+
"metadata": dict,
44+
"tags": list,
45+
"security_groups": list,
46+
"os-extended-volumes:volumes_attached": list,
47+
"OS-EXT-STS:vm_state": str,
48+
"OS-EXT-STS:task_state": str,
49+
"OS-EXT-STS:power_state": int,
50+
"OS-EXT-AZ:availability_zone": str,
51+
"OS-EXT-SRV-ATTR:host": str,
52+
"OS-EXT-SRV-ATTR:hypervisor_hostname": str,
53+
"OS-EXT-SRV-ATTR:instance_name": str,
54+
"OS-DCF:diskConfig": str,
55+
"OS-SRV-USG:launched_at": str,
56+
"OS-SRV-USG:terminated_at": str,
57+
},
58+
total=False,
59+
)

orca_cli/services/__init__.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
"""Service layer — high-level operations on OpenStack resources.
2+
3+
Each service wraps an OrcaClient and exposes typed methods that map to
4+
the underlying API endpoints. Command handlers in ``orca_cli/commands/``
5+
delegate to a service rather than calling ``client.get/post/...``
6+
directly; the trade-offs are documented in ADR-0007.
7+
8+
Services are introduced incrementally — only the resources whose
9+
command modules have been migrated have a corresponding service yet.
10+
"""

orca_cli/services/server.py

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
"""High-level operations on Nova servers."""
2+
3+
from __future__ import annotations
4+
5+
from typing import Any
6+
7+
from orca_cli.core.client import OrcaClient
8+
from orca_cli.models.server import Server
9+
10+
11+
class ServerService:
12+
"""Typed wrapper around the Nova ``/servers`` endpoints.
13+
14+
Owns URL construction; commands import this service instead of
15+
building ``f"{client.compute_url}/servers/..."`` strings themselves.
16+
Retry, auth, and rate-limit handling live in OrcaClient — the
17+
service is purely a translation layer between the Nova API and the
18+
typed model.
19+
"""
20+
21+
def __init__(self, client: OrcaClient) -> None:
22+
self._client = client
23+
self._base = f"{client.compute_url}/servers"
24+
25+
# ── reads ──────────────────────────────────────────────────────────
26+
27+
def find(self, limit: int = 50) -> list[Server]:
28+
"""Return up to ``limit`` servers with their detail payload.
29+
30+
Named ``find`` rather than ``list`` to avoid shadowing the
31+
builtin within the class scope (mypy then can't resolve
32+
``list[Server]`` annotations on sibling methods).
33+
"""
34+
data = self._client.get(f"{self._base}/detail", params={"limit": limit})
35+
return data.get("servers", [])
36+
37+
def find_all(self, page_size: int = 1000) -> list[Server]:
38+
"""Paginate through every server in the project (no silent cap)."""
39+
return self._client.paginate(f"{self._base}/detail", "servers",
40+
page_size=page_size)
41+
42+
def get(self, server_id: str) -> Server:
43+
"""Fetch one server by ID. Raises APIError if not found."""
44+
data = self._client.get(f"{self._base}/{server_id}")
45+
return data.get("server", data)
46+
47+
# ── writes ─────────────────────────────────────────────────────────
48+
49+
def delete(self, server_id: str) -> None:
50+
"""Issue an asynchronous delete; the server transitions to
51+
DELETED state."""
52+
self._client.delete(f"{self._base}/{server_id}")
53+
54+
def action(self, server_id: str, body: dict[str, Any]) -> None:
55+
"""POST a Nova action verb (e.g. ``{"reboot": {"type": "SOFT"}}``)."""
56+
self._client.post(f"{self._base}/{server_id}/action", json=body)
57+
58+
def start(self, server_id: str) -> None:
59+
self.action(server_id, {"os-start": None})
60+
61+
def stop(self, server_id: str) -> None:
62+
self.action(server_id, {"os-stop": None})
63+
64+
def reboot(self, server_id: str, *, hard: bool = False) -> None:
65+
self.action(server_id, {"reboot": {"type": "HARD" if hard else "SOFT"}})

0 commit comments

Comments
 (0)