From feffa7d751ee5e93933803fef1e29505e30d7f8c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 7 May 2026 09:27:50 +0000 Subject: [PATCH 01/14] chore: update upstream OpenAPI spec 2026-05-07 --- api/upstream-openapi.json | 340 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 340 insertions(+) diff --git a/api/upstream-openapi.json b/api/upstream-openapi.json index 90363fb..052f66d 100644 --- a/api/upstream-openapi.json +++ b/api/upstream-openapi.json @@ -513,6 +513,121 @@ ] } }, + "/api/v2/projects/{project_name}/deployments": { + "get": { + "tags": [ + "v2", + "v2", + "deployments" + ], + "summary": "List Deployments V2", + "description": "List deployments in a project with components, images, and computed URLs.\n\nReturns only deployments targeting the current cluster.\n\nHeaders:\n X-API-Key: The API key for the project (required)", + "operationId": "list_deployments_v2_api_v2_projects__project_name__deployments_get", + "parameters": [ + { + "name": "project_name", + "in": "path", + "required": true, + "schema": { + "type": "string", + "title": "Project Name" + } + } + ], + "responses": { + "200": { + "description": "Successful Response", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DeploymentListResponse" + } + } + } + }, + "404": { + "description": "Not found" + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + } + } + }, + "security": [ + { + "APIKeyHeader": [] + } + ] + } + }, + "/api/v2/projects/{project_name}/deployments/{deployment_name}": { + "get": { + "tags": [ + "v2", + "v2", + "deployments" + ], + "summary": "Get Deployment V2", + "description": "Get a single deployment with components, images, and computed URLs.\n\nReturns the current state of a deployment as defined in the project file,\nwith computed public URLs for components that have publish-on-web.\n\nHeaders:\n X-API-Key: The API key for the project (required)", + "operationId": "get_deployment_v2_api_v2_projects__project_name__deployments__deployment_name__get", + "parameters": [ + { + "name": "project_name", + "in": "path", + "required": true, + "schema": { + "type": "string", + "title": "Project Name" + } + }, + { + "name": "deployment_name", + "in": "path", + "required": true, + "schema": { + "type": "string", + "title": "Deployment Name" + } + } + ], + "responses": { + "200": { + "description": "Successful Response", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DeploymentDetail" + } + } + } + }, + "404": { + "description": "Not found" + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + } + } + }, + "security": [ + { + "APIKeyHeader": [] + } + ] + } + }, "/api/v2/projects/{project_name}/deployments/{deployment_name}/:clone-bucket": { "post": { "tags": [ @@ -6261,6 +6376,151 @@ "title": "DeploymentBackupResponse", "description": "Response for combined deployment backup operations (PVCs, databases, buckets)." }, + "DeploymentComponentDetail": { + "properties": { + "reference": { + "type": "string", + "title": "Reference", + "description": "Component name reference" + }, + "image": { + "type": "string", + "title": "Image", + "description": "Container image URL" + } + }, + "type": "object", + "required": [ + "reference", + "image" + ], + "title": "DeploymentComponentDetail", + "description": "Component within a deployment, including image reference." + }, + "DeploymentDetail": { + "properties": { + "name": { + "type": "string", + "title": "Name", + "description": "Deployment name" + }, + "project": { + "type": "string", + "title": "Project", + "description": "Project name" + }, + "cluster": { + "type": "string", + "title": "Cluster", + "description": "Target cluster" + }, + "namespace": { + "type": "string", + "title": "Namespace", + "description": "Kubernetes namespace" + }, + "subdomain": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Subdomain", + "description": "DNS subdomain override" + }, + "components": { + "items": { + "$ref": "#/components/schemas/DeploymentComponentDetail" + }, + "type": "array", + "title": "Components", + "description": "Component references" + }, + "urls": { + "additionalProperties": { + "type": "string" + }, + "type": "object", + "title": "Urls", + "description": "Computed public URLs, keyed by component name" + }, + "status": { + "$ref": "#/components/schemas/DeploymentStatus", + "description": "Overall deployment state. Always present; check value to render." + }, + "sync_revision": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Sync Revision", + "description": "Git revision (full SHA) the cluster last reconciled; null if never reconciled" + }, + "last_synced_at": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Last Synced At", + "description": "ISO timestamp of the last reconciliation attempt against git, regardless of outcome. Combine with status to know whether that attempt succeeded; for a Degraded deployment this can be the time of a failed sync, not a healthy one. Null if no reconciliation has ever happened." + }, + "errors": { + "items": { + "$ref": "#/components/schemas/StatusError" + }, + "type": "array", + "title": "Errors", + "description": "Cluster-side error entries; populated only when status indicates a problem (Degraded, OutOfSync, Suspended, Missing). Empty otherwise." + } + }, + "type": "object", + "required": [ + "name", + "project", + "cluster", + "namespace", + "status" + ], + "title": "DeploymentDetail", + "description": "Full deployment state as returned by the GET endpoints." + }, + "DeploymentListResponse": { + "properties": { + "project": { + "type": "string", + "title": "Project" + }, + "cluster": { + "type": "string", + "title": "Cluster" + }, + "deployments": { + "items": { + "$ref": "#/components/schemas/DeploymentDetail" + }, + "type": "array", + "title": "Deployments" + } + }, + "type": "object", + "required": [ + "project", + "cluster" + ], + "title": "DeploymentListResponse", + "description": "Response for GET /projects/{project_name}/deployments." + }, "DeploymentRestoreRequest": { "properties": { "resource_type": { @@ -6412,6 +6672,36 @@ "status": "success" } }, + "DeploymentStatus": { + "type": "string", + "enum": [ + "Healthy", + "Degraded", + "Progressing", + "OutOfSync", + "Suspended", + "Missing", + "Pending", + "Unavailable", + "Unknown" + ], + "title": "DeploymentStatus", + "description": "Overall deployment state.\n\nA single enum covering everything a caller wants to switch on. Argo's\ntwo orthogonal dimensions (sync, health) are collapsed using a\nworst-of-both priority: Degraded/Suspended/Missing > OutOfSync >\nProgressing > Healthy. Pending and Unavailable are *our* states for\n\"we have no data,\" distinct from Argo's own Unknown." + }, + "ErrorCategory": { + "type": "string", + "enum": [ + "ImagePull", + "CrashLoop", + "OutOfMemory", + "HealthCheck", + "SyncFailed", + "ComparisonError", + "Unknown" + ], + "title": "ErrorCategory", + "description": "Programmatic categorization of a cluster error. Use ``message`` for the raw text.\n\nCategories are intentionally broader than literal Kubernetes reasons (e.g.\n``ImagePull`` covers ``ImagePullBackOff``, ``ErrImagePull``, manifest-unknown\npulls, etc.) so app-level categories can be added later without breaking\nconsumers tied to specific K8s state names." + }, "HTTPValidationError": { "properties": { "detail": { @@ -7029,6 +7319,56 @@ "title": "SnapshotInfoModel", "description": "Information about a Kopia snapshot." }, + "StatusError": { + "properties": { + "resource": { + "type": "string", + "title": "Resource", + "description": "Kind/name (e.g. 'Pod/frontend-abc') or 'Event/' for events" + }, + "message": { + "type": "string", + "title": "Message", + "description": "Raw cluster message \u2014 for automation, regex matching, correlation" + }, + "category": { + "$ref": "#/components/schemas/ErrorCategory", + "description": "Programmatic category for filtering, grouping, colorizing" + }, + "explanation": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Explanation", + "description": "Human-friendly explanation of the category and what to do next; null when the category has no canned guidance (e.g. Unknown)" + }, + "timestamp": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Timestamp", + "description": "ISO timestamp if known" + } + }, + "type": "object", + "required": [ + "resource", + "message", + "category" + ], + "title": "StatusError", + "description": "A single error or warning entry surfaced from the cluster." + }, "StorageAction": { "properties": { "action": { From 178ea7ace076367e43278a80ea97788e74f4eb96 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:01:02 +0200 Subject: [PATCH 02/14] feat: use v2 deployment read endpoints Upstream RIG-Cluster shipped GET /v2/projects/{p}/deployments and GET /v2/projects/{p}/deployments/{d} (closes the structural problem flagged in RijksICTGilde/RIG-Cluster#51). The CLI no longer has to fuse /logs and /tasks to reconstruct deployment state. Client: - add ZadClient.list_deployments_v2 and get_deployment_v2 wrappers - list_deployments and describe_deployment now prefer the v2 endpoints and fall back to the legacy logs+tasks fusion on 404, so older Operations Manager deployments keep working - legacy code paths preserved as _list_deployments_legacy and _describe_deployment_legacy Models: - add DeploymentStatus and ErrorCategory string enums - add DeploymentDetail, DeploymentListResponse, DeploymentComponentDetail, StatusError pydantic models for the new response shapes Describe rendering: - show Status (color-coded), Revision (short SHA), Last sync - when Status indicates a problem, show an Errors table with Category/Resource/Message and a deduplicated explanation footer - hide the K8s Deployment column in the v2 path (the field doesn't exist there); legacy path still populates it Tests: - new respx tests for the v2 happy paths and the 404 fallback - existing legacy-path tests now mock a 404 on the v2 endpoint to exercise the same fallback code --- src/zad_cli/api/client.py | 106 ++++++++++++++++---- src/zad_cli/api/models.py | 68 +++++++++++++ src/zad_cli/commands/deployment.py | 45 ++++++++- tests/test_client.py | 156 ++++++++++++++++++++++++++++- uv.lock | 2 +- 5 files changed, 352 insertions(+), 25 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index a37a27a..d1d9d20 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -438,7 +438,27 @@ def get_logs( response = self._request("GET", f"/logs/{project}", params=params) return response.json() - # --- Project introspection (derived from logs + tasks + subdomains) --- + # --- V2 deployment read endpoints --- + + def list_deployments_v2(self, project: str) -> dict: + """Read all deployments in a project from the v2 read endpoint. + + Returns the raw `DeploymentListResponse` shape (project, cluster, + deployments[]). Each deployment carries status, sync_revision, + last_synced_at, errors, components, and urls. + """ + response = self._request("GET", f"/v2/projects/{project}/deployments") + return response.json() + + def get_deployment_v2(self, project: str, deployment: str) -> dict: + """Read a single deployment from the v2 read endpoint. + + Returns the raw `DeploymentDetail` shape. + """ + response = self._request("GET", f"/v2/projects/{project}/deployments/{deployment}") + return response.json() + + # --- Project introspection --- def resolve_namespace(self, project: str, deployment: str) -> str: """Resolve a deployment name to its Kubernetes namespace.""" @@ -451,9 +471,69 @@ def resolve_namespace(self, project: str, deployment: str) -> str: def list_deployments(self, project: str) -> list[dict]: """List all deployments and their components in a project. - Uses the logs endpoint with limit=0 to discover active deployments, - and tasks to determine deployment status. + Prefers the v2 read endpoint. Falls back to the logs+tasks fusion + when the upstream Operations Manager predates the read endpoint. + Returns the legacy shape: deployment, project, namespace, + components (list of names), status. """ + try: + data = self.list_deployments_v2(project) + except ZadApiError as e: + if e.status_code == 404: + return self._list_deployments_legacy(project) + raise + + rows: list[dict] = [] + for dep in data.get("deployments", []): + rows.append( + { + "deployment": dep["name"], + "project": dep.get("project", project), + "namespace": dep.get("namespace", ""), + "components": [c["reference"] for c in dep.get("components", [])], + "status": dep.get("status", "Unknown"), + "urls": dep.get("urls", {}), + "sync_revision": dep.get("sync_revision"), + "last_synced_at": dep.get("last_synced_at"), + "errors": dep.get("errors", []), + } + ) + return rows + + def describe_deployment(self, project: str, deployment: str) -> dict: + """Get detailed info about a deployment. + + Prefers the v2 read endpoint. Falls back to the logs+tasks fusion + on 404. Returns the legacy shape augmented with status, sync_revision, + last_synced_at, and errors when the v2 endpoint is available. + """ + try: + dep = self.get_deployment_v2(project, deployment) + except ZadApiError as e: + if e.status_code == 404: + # Either the deployment doesn't exist or the upstream lacks + # the read endpoint. The legacy path raises its own 404 only + # if the deployment is genuinely unknown. + return self._describe_deployment_legacy(project, deployment) + raise + + return { + "deployment": dep["name"], + "project": dep.get("project", project), + "namespace": dep.get("namespace", ""), + "components": [ + {"name": c["reference"], "image": c["image"], "k8s_deployment": ""} for c in dep.get("components", []) + ], + "urls": dep.get("urls", {}), + "status": dep.get("status", "Unknown"), + "sync_revision": dep.get("sync_revision"), + "last_synced_at": dep.get("last_synced_at"), + "errors": dep.get("errors", []), + } + + # --- Legacy fallbacks (logs+tasks fusion) --- + + def _list_deployments_legacy(self, project: str) -> list[dict]: response = self._request("GET", f"/logs/{project}", params={"limit": "0"}) data = response.json() @@ -470,7 +550,6 @@ def list_deployments(self, project: str) -> list[dict]: } deployments[dep_name]["components"].append(entry["component"]) - # Enrich with last known task status per deployment task_response = self._request("GET", "/tasks", params={"project_name": project}) tasks = task_response.json().get("tasks", []) seen: set[str] = set() @@ -487,9 +566,7 @@ def list_deployments(self, project: str) -> list[dict]: return list(deployments.values()) - def describe_deployment(self, project: str, deployment: str) -> dict: - """Get detailed info about a deployment by combining logs and task history.""" - # Get components from logs + def _describe_deployment_legacy(self, project: str, deployment: str) -> dict: response = self._request("GET", f"/logs/{project}", params={"deployment": deployment, "limit": "0"}) log_data = response.json() @@ -497,17 +574,9 @@ def describe_deployment(self, project: str, deployment: str) -> dict: namespace = "" for entry in log_data.get("results", []): namespace = entry.get("namespace", namespace) - components.append( - { - "name": entry["component"], - "k8s_deployment": entry.get("k8s_deployment", ""), - } - ) + components.append({"name": entry["component"], "k8s_deployment": entry.get("k8s_deployment", "")}) - # Upstream has no `GET /projects/{p}/deployments/{d}` endpoint, so URLs - # and image refs are reconstructed from completed task history. Filter - # server-side to avoid paging all project tasks. - urls = {} + urls: dict[str, str] = {} images: dict[str, str] = {} task_response = self._request( "GET", @@ -524,19 +593,16 @@ def describe_deployment(self, project: str, deployment: str) -> dict: continue if dep_info.get("name") != deployment: continue - # Get URLs (prefer most recent) urls_data = result.get("urls") or {} dep_data = urls_data.get(deployment) or {} dep_urls = dep_data.get("urls", {}) if isinstance(dep_data, dict) else {} if dep_urls and not urls: urls = dep_urls - # Accumulate images from all tasks (most recent wins per component) for comp in dep_info.get("components", []): ref = comp.get("reference", "") if ref and ref not in images: images[ref] = comp.get("image", "") - # Merge image info into components for comp in components: comp["image"] = images.get(comp["name"], "") diff --git a/src/zad_cli/api/models.py b/src/zad_cli/api/models.py index 6379457..0773163 100644 --- a/src/zad_cli/api/models.py +++ b/src/zad_cli/api/models.py @@ -3,6 +3,7 @@ from __future__ import annotations import re +from enum import StrEnum from pydantic import BaseModel, field_validator @@ -156,3 +157,70 @@ class TaskStatus(BaseModel): progress_percent: int | None = None result: dict | None = None error_message: str | None = None + + +class DeploymentStatus(StrEnum): + """Overall deployment state from GET /v2/.../deployments.""" + + HEALTHY = "Healthy" + DEGRADED = "Degraded" + PROGRESSING = "Progressing" + OUT_OF_SYNC = "OutOfSync" + SUSPENDED = "Suspended" + MISSING = "Missing" + PENDING = "Pending" + UNAVAILABLE = "Unavailable" + UNKNOWN = "Unknown" + + +class ErrorCategory(StrEnum): + """Programmatic category for a cluster error entry.""" + + IMAGE_PULL = "ImagePull" + CRASH_LOOP = "CrashLoop" + OUT_OF_MEMORY = "OutOfMemory" + HEALTH_CHECK = "HealthCheck" + SYNC_FAILED = "SyncFailed" + COMPARISON_ERROR = "ComparisonError" + UNKNOWN = "Unknown" + + +class StatusError(BaseModel): + """A single cluster-side error or warning surfaced on a deployment.""" + + resource: str + message: str + category: ErrorCategory + explanation: str | None = None + timestamp: str | None = None + + +class DeploymentComponentDetail(BaseModel): + """Component within a deployment as returned by the v2 read endpoints.""" + + reference: str + image: str + + +class DeploymentDetail(BaseModel): + """Full deployment state from GET /v2/projects/{p}/deployments/{d}.""" + + name: str + project: str + cluster: str + namespace: str + subdomain: str | None = None + components: list[DeploymentComponentDetail] = [] + urls: dict[str, str] = {} + status: DeploymentStatus + sync_revision: str | None = None + last_synced_at: str | None = None + errors: list[StatusError] = [] + + +class DeploymentListResponse(BaseModel): + """Response from GET /v2/projects/{p}/deployments.""" + + project: str + cluster: str + deployments: list[DeploymentDetail] = [] diff --git a/src/zad_cli/commands/deployment.py b/src/zad_cli/commands/deployment.py index a49ab0c..55bb9a5 100644 --- a/src/zad_cli/commands/deployment.py +++ b/src/zad_cli/commands/deployment.py @@ -85,6 +85,15 @@ def describe( console.print(f"[bold]Project:[/bold] {result['project']}") console.print(f"[bold]Namespace:[/bold] {result['namespace']}") + status = result.get("status") + if status: + color = _status_color(status) + console.print(f"[bold]Status:[/bold] [{color}]{status}[/{color}]") + if result.get("sync_revision"): + console.print(f"[bold]Revision:[/bold] {result['sync_revision'][:12]}") + if result.get("last_synced_at"): + console.print(f"[bold]Last sync:[/bold] {result['last_synced_at']}") + if result.get("urls"): console.print("\n[bold]URLs:[/bold]") for comp_name, url in result["urls"].items(): @@ -93,22 +102,54 @@ def describe( console.print() has_images = any(comp.get("image") for comp in result["components"]) + has_k8s = any(comp.get("k8s_deployment") for comp in result["components"]) table = Table(title="Components", show_header=True) table.add_column("Name", style="bold cyan") if has_images: table.add_column("Image") - table.add_column("K8s Deployment") + if has_k8s: + table.add_column("K8s Deployment") for comp in result["components"]: row = [comp["name"]] if has_images: row.append(comp.get("image", "")) - row.append(comp.get("k8s_deployment", "")) + if has_k8s: + row.append(comp.get("k8s_deployment", "")) table.add_row(*row) console.print(table) + errors = result.get("errors") or [] + if errors: + err_table = Table(title="Errors", show_header=True, header_style="bold red") + err_table.add_column("Category", style="bold") + err_table.add_column("Resource") + err_table.add_column("Message") + for err in errors: + err_table.add_row(err.get("category", ""), err.get("resource", ""), err.get("message", "")) + console.print(err_table) + + seen_explanations: set[str] = set() + for err in errors: + cat = err.get("category", "") + explanation = err.get("explanation") + if explanation and cat not in seen_explanations: + seen_explanations.add(cat) + console.print(f" [dim]{cat}: {explanation}[/dim]") + + +def _status_color(status: str) -> str: + """Color for a DeploymentStatus enum value.""" + if status == "Healthy": + return "green" + if status in ("Degraded", "Missing", "OutOfSync"): + return "red" + if status in ("Progressing", "Pending"): + return "yellow" + return "dim" + @app.command() @handle_api_errors diff --git a/tests/test_client.py b/tests/test_client.py index 97c882d..bd8e1b7 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -148,7 +148,10 @@ def test_api_key_header(client): @respx.mock def test_describe_deployment_filters_tasks_server_side(client): - """describe_deployment must narrow the /tasks query to the target deployment.""" + """Legacy fallback: when the v2 endpoint 404s, describe must narrow /tasks server-side.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -193,7 +196,10 @@ def test_describe_deployment_filters_tasks_server_side(client): @respx.mock def test_describe_deployment_ignores_tasks_with_mismatched_name(client): - """If the server filter ever leaks a mismatched task, the client guard must drop it.""" + """Legacy fallback: if the server filter ever leaks a mismatched task, the client guard must drop it.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -228,3 +234,149 @@ def test_describe_deployment_ignores_tasks_with_mismatched_name(client): assert result["urls"] == {} assert result["components"][0]["image"] == "" + + +@respx.mock +def test_describe_deployment_uses_v2_endpoint(client): + """describe_deployment prefers the v2 read endpoint when available.""" + route = respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( + return_value=httpx.Response( + 200, + json={ + "name": "staging", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-staging", + "subdomain": None, + "components": [{"reference": "web", "image": "ghcr.io/org/web:v2"}], + "urls": {"web": "https://staging.example.com"}, + "status": "Healthy", + "sync_revision": "abc123def456", + "last_synced_at": "2026-05-07T09:00:00Z", + "errors": [], + }, + ) + ) + + result = client.describe_deployment("my-project", "staging") + + assert route.called + assert result["deployment"] == "staging" + assert result["namespace"] == "ns-staging" + assert result["status"] == "Healthy" + assert result["sync_revision"] == "abc123def456" + assert result["urls"] == {"web": "https://staging.example.com"} + assert result["components"][0]["image"] == "ghcr.io/org/web:v2" + + +@respx.mock +def test_describe_deployment_surfaces_errors(client): + """Degraded deployment: errors[] must come through with category and explanation.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( + return_value=httpx.Response( + 200, + json={ + "name": "staging", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-staging", + "components": [{"reference": "web", "image": "ghcr.io/org/web:bad"}], + "urls": {}, + "status": "Degraded", + "sync_revision": "deadbeefcafe", + "last_synced_at": "2026-05-07T08:00:00Z", + "errors": [ + { + "resource": "Pod/web-7c9d8f-xxxxx", + "message": "Back-off pulling image ghcr.io/org/web:bad", + "category": "ImagePull", + "explanation": "Container image cannot be pulled. Check tag and registry credentials.", + "timestamp": "2026-05-07T08:05:00Z", + } + ], + }, + ) + ) + + result = client.describe_deployment("my-project", "staging") + + assert result["status"] == "Degraded" + assert len(result["errors"]) == 1 + assert result["errors"][0]["category"] == "ImagePull" + + +@respx.mock +def test_list_deployments_uses_v2_endpoint(client): + """list_deployments prefers the v2 read endpoint and exposes the legacy shape.""" + route = respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response( + 200, + json={ + "project": "my-project", + "cluster": "odcn-test", + "deployments": [ + { + "name": "staging", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-staging", + "components": [{"reference": "web", "image": "ghcr.io/org/web:v1"}], + "urls": {"web": "https://staging.example.com"}, + "status": "Healthy", + "sync_revision": "abc", + "last_synced_at": "2026-05-07T09:00:00Z", + "errors": [], + }, + { + "name": "production", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-prod", + "components": [ + {"reference": "web", "image": "ghcr.io/org/web:v1"}, + {"reference": "api", "image": "ghcr.io/org/api:v1"}, + ], + "urls": {}, + "status": "Degraded", + "errors": [], + }, + ], + }, + ) + ) + + rows = client.list_deployments("my-project") + + assert route.called + assert len(rows) == 2 + assert rows[0]["deployment"] == "staging" + assert rows[0]["components"] == ["web"] + assert rows[0]["status"] == "Healthy" + assert rows[1]["deployment"] == "production" + assert rows[1]["components"] == ["web", "api"] + assert rows[1]["status"] == "Degraded" + + +@respx.mock +def test_list_deployments_falls_back_on_404(client): + """When the v2 endpoint 404s (older upstream), list_deployments uses the legacy fusion.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/logs/my-project").mock( + return_value=httpx.Response( + 200, + json={ + "results": [ + {"component": "web", "deployment": "staging", "namespace": "ns-staging"}, + ] + }, + ) + ) + respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) + + rows = client.list_deployments("my-project") + + assert len(rows) == 1 + assert rows[0]["deployment"] == "staging" + assert rows[0]["status"] == "Active" diff --git a/uv.lock b/uv.lock index b140a6e..c5f9be0 100644 --- a/uv.lock +++ b/uv.lock @@ -553,7 +553,7 @@ wheels = [ [[package]] name = "zad-cli" -version = "0.3.0" +version = "0.4.0" source = { editable = "." } dependencies = [ { name = "httpx" }, From 2e1d2ac040e4ad0da605830d468304ee4d7517d9 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:08:17 +0200 Subject: [PATCH 03/14] fix: address review on PR #23 - describe_deployment now propagates a real 404 when the v2 list endpoint works but get-one 404s (deployment is genuinely missing). Previously the legacy fallback would silently return empty data. Probe by calling list_deployments_v2 to disambiguate "deployment missing" from "endpoint not registered on this upstream". - _status_color: Suspended joins the red tier alongside Degraded, Missing, OutOfSync per upstream's documented severity ordering. - backwards-compat baseline: register list_deployments_v2 and get_deployment_v2 as expected public methods. --- src/zad_cli/api/client.py | 18 ++++++++--- src/zad_cli/commands/deployment.py | 2 +- tests/test_backwards_compat.py | 4 +++ tests/test_client.py | 51 +++++++++++++++++++++++++++++- 4 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index d1d9d20..92656e8 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -504,17 +504,25 @@ def describe_deployment(self, project: str, deployment: str) -> dict: """Get detailed info about a deployment. Prefers the v2 read endpoint. Falls back to the logs+tasks fusion - on 404. Returns the legacy shape augmented with status, sync_revision, + only when the upstream Operations Manager predates the read endpoint. + Returns the legacy shape augmented with status, sync_revision, last_synced_at, and errors when the v2 endpoint is available. """ try: dep = self.get_deployment_v2(project, deployment) except ZadApiError as e: if e.status_code == 404: - # Either the deployment doesn't exist or the upstream lacks - # the read endpoint. The legacy path raises its own 404 only - # if the deployment is genuinely unknown. - return self._describe_deployment_legacy(project, deployment) + # Disambiguate "deployment not found" from "endpoint not + # registered on this upstream". If the list endpoint also + # 404s, fall back to the legacy path; otherwise the + # deployment really is missing and we propagate the 404. + try: + self.list_deployments_v2(project) + except ZadApiError as list_err: + if list_err.status_code == 404: + return self._describe_deployment_legacy(project, deployment) + raise + raise ZadApiError(404, f"Deployment '{deployment}' not found in project '{project}'") from e raise return { diff --git a/src/zad_cli/commands/deployment.py b/src/zad_cli/commands/deployment.py index 55bb9a5..a601407 100644 --- a/src/zad_cli/commands/deployment.py +++ b/src/zad_cli/commands/deployment.py @@ -144,7 +144,7 @@ def _status_color(status: str) -> str: """Color for a DeploymentStatus enum value.""" if status == "Healthy": return "green" - if status in ("Degraded", "Missing", "OutOfSync"): + if status in ("Degraded", "Missing", "OutOfSync", "Suspended"): return "red" if status in ("Progressing", "Pending"): return "yellow" diff --git a/tests/test_backwards_compat.py b/tests/test_backwards_compat.py index ac5e654..49f9972 100644 --- a/tests/test_backwards_compat.py +++ b/tests/test_backwards_compat.py @@ -138,11 +138,13 @@ def test_cli_commands_not_removed(): "delete_project", "delete_snapshot", "describe_deployment", + "get_deployment_v2", "get_logs", "get_task", "health", "list_backup_runs", "list_deployments", + "list_deployments_v2", "list_projects", "list_snapshots", "list_subdomains", @@ -207,11 +209,13 @@ def test_client_public_methods_not_removed(): "delete_project": 1, "delete_snapshot": 3, "describe_deployment": 2, + "get_deployment_v2": 2, "get_logs": 1, "get_task": 1, "health": 0, "list_backup_runs": 2, "list_deployments": 1, + "list_deployments_v2": 1, "list_projects": 0, "list_snapshots": 2, "list_subdomains": 0, diff --git a/tests/test_client.py b/tests/test_client.py index bd8e1b7..df60e53 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -148,10 +148,13 @@ def test_api_key_header(client): @respx.mock def test_describe_deployment_filters_tasks_server_side(client): - """Legacy fallback: when the v2 endpoint 404s, describe must narrow /tasks server-side.""" + """Legacy fallback: when both v2 endpoints 404, describe must narrow /tasks server-side.""" respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -200,6 +203,9 @@ def test_describe_deployment_ignores_tasks_with_mismatched_name(client): respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -357,6 +363,49 @@ def test_list_deployments_uses_v2_endpoint(client): assert rows[1]["status"] == "Degraded" +@respx.mock +def test_describe_deployment_propagates_404_when_v2_endpoint_exists(client): + """When the v2 list endpoint works but the get endpoint 404s, the deployment is genuinely missing.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments/missing").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response( + 200, + json={"project": "my-project", "cluster": "odcn-test", "deployments": []}, + ) + ) + + with pytest.raises(ZadApiError) as exc_info: + client.describe_deployment("my-project", "missing") + + assert exc_info.value.status_code == 404 + assert "missing" in str(exc_info.value) + + +@respx.mock +def test_describe_deployment_falls_back_on_old_upstream(client): + """When both v2 endpoints 404, the upstream lacks read endpoints; use the legacy fusion.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/logs/my-project").mock( + return_value=httpx.Response( + 200, + json={"results": [{"component": "web", "deployment": "staging", "namespace": "ns-staging"}]}, + ) + ) + respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) + + result = client.describe_deployment("my-project", "staging") + + assert result["deployment"] == "staging" + assert result["namespace"] == "ns-staging" + + @respx.mock def test_list_deployments_falls_back_on_404(client): """When the v2 endpoint 404s (older upstream), list_deployments uses the legacy fusion.""" From 5c99d9d1e89966ffaf0668f761db8766cd7b73cf Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:16:56 +0200 Subject: [PATCH 04/14] fix: address second review on PR #23 - list_deployments now disambiguates "project not found" from "v2 endpoint not registered on this upstream", mirroring the describe_deployment logic. Probes via list_projects (available on every upstream version) before falling back to the legacy fusion. - "Last sync" label renamed to "Last sync attempt": per upstream spec, the timestamp is the most recent reconciliation attempt regardless of outcome, so a Degraded deployment with a recent failed sync no longer reads as if it last synced cleanly. - Move _status_color above describe (helpers before callers). --- src/zad_cli/api/client.py | 13 +++++++++++-- src/zad_cli/commands/deployment.py | 27 +++++++++++++++------------ tests/test_client.py | 24 ++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index 92656e8..929399f 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -472,7 +472,7 @@ def list_deployments(self, project: str) -> list[dict]: """List all deployments and their components in a project. Prefers the v2 read endpoint. Falls back to the logs+tasks fusion - when the upstream Operations Manager predates the read endpoint. + only when the upstream Operations Manager predates the read endpoint. Returns the legacy shape: deployment, project, namespace, components (list of names), status. """ @@ -480,7 +480,16 @@ def list_deployments(self, project: str) -> list[dict]: data = self.list_deployments_v2(project) except ZadApiError as e: if e.status_code == 404: - return self._list_deployments_legacy(project) + # Disambiguate "project not found" from "endpoint not + # registered on this upstream". list_projects works on every + # upstream version, so it gives an authoritative answer. + try: + projects = self.list_projects() + except ZadApiError: + return self._list_deployments_legacy(project) + if any(p.get("name") == project for p in projects): + return self._list_deployments_legacy(project) + raise ZadApiError(404, f"Project '{project}' not found") from e raise rows: list[dict] = [] diff --git a/src/zad_cli/commands/deployment.py b/src/zad_cli/commands/deployment.py index a601407..084720f 100644 --- a/src/zad_cli/commands/deployment.py +++ b/src/zad_cli/commands/deployment.py @@ -56,6 +56,17 @@ def list_deployments(ctx: typer.Context) -> None: ) +def _status_color(status: str) -> str: + """Color for a DeploymentStatus enum value.""" + if status == "Healthy": + return "green" + if status in ("Degraded", "Missing", "OutOfSync", "Suspended"): + return "red" + if status in ("Progressing", "Pending"): + return "yellow" + return "dim" + + @app.command() @handle_api_errors def describe( @@ -92,7 +103,10 @@ def describe( if result.get("sync_revision"): console.print(f"[bold]Revision:[/bold] {result['sync_revision'][:12]}") if result.get("last_synced_at"): - console.print(f"[bold]Last sync:[/bold] {result['last_synced_at']}") + # Per upstream spec: this is the last sync *attempt*, not necessarily + # successful. Phrase the label so a Degraded deployment doesn't look + # like it last synced cleanly. + console.print(f"[bold]Last sync attempt:[/bold] {result['last_synced_at']}") if result.get("urls"): console.print("\n[bold]URLs:[/bold]") @@ -140,17 +154,6 @@ def describe( console.print(f" [dim]{cat}: {explanation}[/dim]") -def _status_color(status: str) -> str: - """Color for a DeploymentStatus enum value.""" - if status == "Healthy": - return "green" - if status in ("Degraded", "Missing", "OutOfSync", "Suspended"): - return "red" - if status in ("Progressing", "Pending"): - return "yellow" - return "dim" - - @app.command() @handle_api_errors def create( diff --git a/tests/test_client.py b/tests/test_client.py index df60e53..ad5b4be 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -407,11 +407,14 @@ def test_describe_deployment_falls_back_on_old_upstream(client): @respx.mock -def test_list_deployments_falls_back_on_404(client): - """When the v2 endpoint 404s (older upstream), list_deployments uses the legacy fusion.""" +def test_list_deployments_falls_back_on_old_upstream(client): + """v2 endpoint 404s but the project exists: upstream is old, use legacy fusion.""" respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) + respx.get("https://api.example.com/projects").mock( + return_value=httpx.Response(200, json=[{"name": "my-project"}, {"name": "other"}]) + ) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -429,3 +432,20 @@ def test_list_deployments_falls_back_on_404(client): assert len(rows) == 1 assert rows[0]["deployment"] == "staging" assert rows[0]["status"] == "Active" + + +@respx.mock +def test_list_deployments_propagates_404_when_project_missing(client): + """v2 endpoint 404s and the project is not in list_projects: raise 404, don't silently fall back.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/projects").mock( + return_value=httpx.Response(200, json=[{"name": "other-project"}]) + ) + + with pytest.raises(ZadApiError) as exc_info: + client.list_deployments("my-project") + + assert exc_info.value.status_code == 404 + assert "my-project" in str(exc_info.value) From a4003ae32c1f454b05e930dc9c1b0df633175a60 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:22:09 +0200 Subject: [PATCH 05/14] fix: only swallow 404 from list_projects probe The disambiguation probe in list_deployments was catching every ZadApiError from list_projects, including 401/403/5xx. An expired API key or transient server error would silently route to the legacy fusion path and surface a confusing error from /logs instead of the real cause. Now only 404 means "old upstream"; everything else propagates. --- src/zad_cli/api/client.py | 10 ++++++++-- tests/test_client.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index 929399f..cd0332b 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -483,10 +483,16 @@ def list_deployments(self, project: str) -> list[dict]: # Disambiguate "project not found" from "endpoint not # registered on this upstream". list_projects works on every # upstream version, so it gives an authoritative answer. + # Only swallow a 404 from list_projects (the legacy V1 + # /projects route is universal, but we treat its absence + # as "fall back" rather than masking real errors like + # 401/403/5xx). try: projects = self.list_projects() - except ZadApiError: - return self._list_deployments_legacy(project) + except ZadApiError as list_err: + if list_err.status_code == 404: + return self._list_deployments_legacy(project) + raise if any(p.get("name") == project for p in projects): return self._list_deployments_legacy(project) raise ZadApiError(404, f"Project '{project}' not found") from e diff --git a/tests/test_client.py b/tests/test_client.py index ad5b4be..e2fbf6b 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -434,6 +434,22 @@ def test_list_deployments_falls_back_on_old_upstream(client): assert rows[0]["status"] == "Active" +@respx.mock +def test_list_deployments_propagates_non_404_from_probe(client): + """If list_projects returns 401/403/500, propagate the real error rather than masking it via legacy fallback.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/projects").mock( + return_value=httpx.Response(401, json={"detail": "Unauthorized"}) + ) + + with pytest.raises(ZadApiError) as exc_info: + client.list_deployments("my-project") + + assert exc_info.value.status_code == 401 + + @respx.mock def test_list_deployments_propagates_404_when_project_missing(client): """v2 endpoint 404s and the project is not in list_projects: raise 404, don't silently fall back.""" From af0ef889b173a306884afd56bfa9d5b40d2e7136 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:29:13 +0200 Subject: [PATCH 06/14] fix: extend describe_deployment 404 disambiguation describe_deployment now mirrors list_deployments: when both v2 endpoints 404, it probes list_projects to tell "old upstream" from "project does not exist on this upstream". The old code silently fell back to the legacy logs+tasks fusion in both cases, masking "Project not found" with empty-component results. Both callers go through a shared _project_exists helper which only swallows a 404 from list_projects (treating that as "even /projects is missing, fall back"); 401/403/5xx propagate so the user sees the real cause. New test: describe_deployment raises ZadApiError(404) with a clear "Project '' not found" message when the project is missing. --- src/zad_cli/api/client.py | 48 +++++++++++++++++++++++---------------- tests/test_client.py | 25 +++++++++++++++++++- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index cd0332b..1f86581 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -480,20 +480,7 @@ def list_deployments(self, project: str) -> list[dict]: data = self.list_deployments_v2(project) except ZadApiError as e: if e.status_code == 404: - # Disambiguate "project not found" from "endpoint not - # registered on this upstream". list_projects works on every - # upstream version, so it gives an authoritative answer. - # Only swallow a 404 from list_projects (the legacy V1 - # /projects route is universal, but we treat its absence - # as "fall back" rather than masking real errors like - # 401/403/5xx). - try: - projects = self.list_projects() - except ZadApiError as list_err: - if list_err.status_code == 404: - return self._list_deployments_legacy(project) - raise - if any(p.get("name") == project for p in projects): + if self._project_exists(project): return self._list_deployments_legacy(project) raise ZadApiError(404, f"Project '{project}' not found") from e raise @@ -527,15 +514,18 @@ def describe_deployment(self, project: str, deployment: str) -> dict: dep = self.get_deployment_v2(project, deployment) except ZadApiError as e: if e.status_code == 404: - # Disambiguate "deployment not found" from "endpoint not - # registered on this upstream". If the list endpoint also - # 404s, fall back to the legacy path; otherwise the - # deployment really is missing and we propagate the 404. + # The 404 means one of three things: deployment missing, + # project missing, or v2 endpoint not registered on this + # upstream. Probe list_deployments_v2 to separate the first + # two from the third; then probe list_projects to separate + # the first from the second. try: self.list_deployments_v2(project) except ZadApiError as list_err: if list_err.status_code == 404: - return self._describe_deployment_legacy(project, deployment) + if self._project_exists(project): + return self._describe_deployment_legacy(project, deployment) + raise ZadApiError(404, f"Project '{project}' not found") from e raise raise ZadApiError(404, f"Deployment '{deployment}' not found in project '{project}'") from e raise @@ -554,6 +544,26 @@ def describe_deployment(self, project: str, deployment: str) -> dict: "errors": dep.get("errors", []), } + def _project_exists(self, project: str) -> bool: + """Probe whether a project exists on this upstream. + + Used to disambiguate 404s from list_deployments_v2 / get_deployment_v2. + list_projects works on every upstream version, so it is the + authoritative source. Only treats a 404 from list_projects itself as + "endpoint missing, fall back" — every other error (401/403/5xx) + propagates so the caller sees the real cause. + """ + try: + projects = self.list_projects() + except ZadApiError as err: + if err.status_code == 404: + # /projects itself is missing on this upstream. Best effort + # is to assume the project exists so the legacy fallback + # gets a chance. + return True + raise + return any(p.get("name") == project for p in projects) + # --- Legacy fallbacks (logs+tasks fusion) --- def _list_deployments_legacy(self, project: str) -> list[dict]: diff --git a/tests/test_client.py b/tests/test_client.py index e2fbf6b..e3cedea 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -155,6 +155,7 @@ def test_describe_deployment_filters_tasks_server_side(client): respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) + respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(200, json=[{"name": "my-project"}])) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -206,6 +207,7 @@ def test_describe_deployment_ignores_tasks_with_mismatched_name(client): respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) + respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(200, json=[{"name": "my-project"}])) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -385,13 +387,14 @@ def test_describe_deployment_propagates_404_when_v2_endpoint_exists(client): @respx.mock def test_describe_deployment_falls_back_on_old_upstream(client): - """When both v2 endpoints 404, the upstream lacks read endpoints; use the legacy fusion.""" + """When both v2 endpoints 404 and the project exists, the upstream lacks read endpoints; use legacy.""" respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) + respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(200, json=[{"name": "my-project"}])) respx.get("https://api.example.com/logs/my-project").mock( return_value=httpx.Response( 200, @@ -406,6 +409,26 @@ def test_describe_deployment_falls_back_on_old_upstream(client): assert result["namespace"] == "ns-staging" +@respx.mock +def test_describe_deployment_propagates_404_when_project_missing(client): + """v2 endpoints 404 and the project is not in list_projects: surface 'Project not found'.""" + respx.get("https://api.example.com/v2/projects/missing-project/deployments/staging").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/v2/projects/missing-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/projects").mock( + return_value=httpx.Response(200, json=[{"name": "other-project"}]) + ) + + with pytest.raises(ZadApiError) as exc_info: + client.describe_deployment("missing-project", "staging") + + assert exc_info.value.status_code == 404 + assert "missing-project" in str(exc_info.value) + + @respx.mock def test_list_deployments_falls_back_on_old_upstream(client): """v2 endpoint 404s but the project exists: upstream is old, use legacy fusion.""" From fb917120654d202d8bafe4d99d1448d70d0d0201 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:36:27 +0200 Subject: [PATCH 07/14] fix: address third review on PR #23 Three findings: 1. (Significant) project_status was unconditionally overwriting v2-supplied URLs with empty dicts on modern upstreams that have no recent completed tasks. The task probe is now best-effort: only overrides v2 URLs when it actually finds something. 2. Validate v2 read endpoint responses through pydantic. list_deployments_v2 and get_deployment_v2 now run the response through DeploymentListResponse / DeploymentDetail and re-emit a dict. Upstream schema drift surfaces as a clear pydantic error instead of leaking malformed data downstream. 3. Tests: - new test for project_status preserving v2 URLs when /tasks is empty - new test for the "/projects also 404s" branch (very old upstream) --- src/zad_cli/api/client.py | 23 ++++++++++++------ tests/test_client.py | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index 1f86581..d2ee17c 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -9,7 +9,7 @@ import httpx -from zad_cli.api.models import TaskStatus +from zad_cli.api.models import DeploymentDetail, DeploymentListResponse, TaskStatus class ZadApiError(Exception): @@ -443,20 +443,25 @@ def get_logs( def list_deployments_v2(self, project: str) -> dict: """Read all deployments in a project from the v2 read endpoint. - Returns the raw `DeploymentListResponse` shape (project, cluster, + Returns the `DeploymentListResponse` shape (project, cluster, deployments[]). Each deployment carries status, sync_revision, last_synced_at, errors, components, and urls. + + The response is validated against `DeploymentListResponse` and then + re-serialized to a dict. Validation surfaces upstream schema drift + as a clear pydantic error instead of letting malformed data leak + downstream. """ response = self._request("GET", f"/v2/projects/{project}/deployments") - return response.json() + return DeploymentListResponse.model_validate(response.json()).model_dump(mode="json") def get_deployment_v2(self, project: str, deployment: str) -> dict: """Read a single deployment from the v2 read endpoint. - Returns the raw `DeploymentDetail` shape. + Returns the `DeploymentDetail` shape, validated through pydantic. """ response = self._request("GET", f"/v2/projects/{project}/deployments/{deployment}") - return response.json() + return DeploymentDetail.model_validate(response.json()).model_dump(mode="json") # --- Project introspection --- @@ -671,9 +676,13 @@ def project_status(self, project: str) -> dict: if dep_name not in urls_by_deployment and dep_urls.get("urls"): urls_by_deployment[dep_name] = dep_urls["urls"] - # Enrich deployments with URLs + # Enrich deployments with URLs from tasks, but never clobber URLs the + # v2 endpoint already supplied. On modern upstreams list_deployments + # populates dep["urls"] directly; the task probe is now best-effort + # for legacy upstreams or when a recent task carries newer URLs. for dep in deployments: - dep["urls"] = urls_by_deployment.get(dep["deployment"], {}) + task_urls = urls_by_deployment.get(dep["deployment"], {}) + dep["urls"] = task_urls or dep.get("urls", {}) return { "project": project, diff --git a/tests/test_client.py b/tests/test_client.py index e3cedea..e64b48a 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -457,6 +457,57 @@ def test_list_deployments_falls_back_on_old_upstream(client): assert rows[0]["status"] == "Active" +@respx.mock +def test_project_status_preserves_v2_urls_when_no_recent_tasks(client): + """project_status must not clobber v2-supplied URLs with empty dicts when tasks have nothing to add.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response( + 200, + json={ + "project": "my-project", + "cluster": "odcn-test", + "deployments": [ + { + "name": "staging", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-staging", + "components": [{"reference": "web", "image": "ghcr.io/org/web:v1"}], + "urls": {"web": "https://staging.example.com"}, + "status": "Healthy", + } + ], + }, + ) + ) + respx.get("https://api.example.com/subdomains").mock(return_value=httpx.Response(200, json={"items": []})) + respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) + + result = client.project_status("my-project") + + assert result["deployments"][0]["urls"] == {"web": "https://staging.example.com"} + + +@respx.mock +def test_list_deployments_uses_legacy_when_projects_endpoint_also_404s(client): + """Very old upstream where /projects itself doesn't exist: assume project exists, fall back to legacy.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(404, json={"detail": "not found"})) + respx.get("https://api.example.com/logs/my-project").mock( + return_value=httpx.Response( + 200, json={"results": [{"component": "web", "deployment": "staging", "namespace": "ns-staging"}]} + ) + ) + respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) + + rows = client.list_deployments("my-project") + + assert len(rows) == 1 + assert rows[0]["deployment"] == "staging" + + @respx.mock def test_list_deployments_propagates_non_404_from_probe(client): """If list_projects returns 401/403/500, propagate the real error rather than masking it via legacy fallback.""" From 9480ade74f8c73e17431349d6ef780f3c3c508b7 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:41:59 +0200 Subject: [PATCH 08/14] fix: v2 URLs are authoritative over task-history URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix inverted the priority — task_urls or dep["urls"] gave task URLs priority when both were non-empty. v2 is authoritative when present, so prefer dep["urls"] (the v2-supplied dict) and fall back to the task probe only when v2 returned nothing (legacy upstreams or deployments with no public components). New test: when both v2 and task history carry URLs for the same deployment, the v2 URLs win. --- src/zad_cli/api/client.py | 8 +++----- tests/test_client.py | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index d2ee17c..b600231 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -676,13 +676,11 @@ def project_status(self, project: str) -> dict: if dep_name not in urls_by_deployment and dep_urls.get("urls"): urls_by_deployment[dep_name] = dep_urls["urls"] - # Enrich deployments with URLs from tasks, but never clobber URLs the - # v2 endpoint already supplied. On modern upstreams list_deployments - # populates dep["urls"] directly; the task probe is now best-effort - # for legacy upstreams or when a recent task carries newer URLs. + # Enrich deployments with URLs. v2 is authoritative when present; + # the task probe is a best-effort fallback for legacy upstreams. for dep in deployments: task_urls = urls_by_deployment.get(dep["deployment"], {}) - dep["urls"] = task_urls or dep.get("urls", {}) + dep["urls"] = dep.get("urls", {}) or task_urls return { "project": project, diff --git a/tests/test_client.py b/tests/test_client.py index e64b48a..f259799 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -488,6 +488,49 @@ def test_project_status_preserves_v2_urls_when_no_recent_tasks(client): assert result["deployments"][0]["urls"] == {"web": "https://staging.example.com"} +@respx.mock +def test_project_status_v2_urls_win_over_stale_task_urls(client): + """When both v2 and task history supply URLs, v2 is authoritative — stale task URLs must not win.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response( + 200, + json={ + "project": "my-project", + "cluster": "odcn-test", + "deployments": [ + { + "name": "staging", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-staging", + "components": [{"reference": "web", "image": "ghcr.io/org/web:v2"}], + "urls": {"web": "https://current.example.com"}, + "status": "Healthy", + } + ], + }, + ) + ) + respx.get("https://api.example.com/subdomains").mock(return_value=httpx.Response(200, json={"items": []})) + respx.get("https://api.example.com/tasks").mock( + return_value=httpx.Response( + 200, + json={ + "tasks": [ + { + "status": "completed", + "result": {"urls": {"staging": {"urls": {"web": "https://stale.example.com"}}}}, + } + ] + }, + ) + ) + + result = client.project_status("my-project") + + assert result["deployments"][0]["urls"] == {"web": "https://current.example.com"} + + @respx.mock def test_list_deployments_uses_legacy_when_projects_endpoint_also_404s(client): """Very old upstream where /projects itself doesn't exist: assume project exists, fall back to legacy.""" From d2a0ca507f8ffae05e12cf4b8c7eec68a39c9ad5 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:49:39 +0200 Subject: [PATCH 09/14] fix: address fourth review on PR #23 Four findings: 1. (Significant) ValidationError from pydantic schema mismatch was not caught by @handle_api_errors and would surface as a raw traceback. New _parse_v2_response helper translates ValidationError to ZadApiError(502, "Unexpected API response shape: ...") so the existing error rendering path handles upstream schema drift cleanly. This becomes load-bearing as upstream adds new DeploymentStatus or ErrorCategory enum values without a breaking-change label. 2. (Minor) project_status URL merge now uses presence rather than truthiness. v2 returning {} legitimately means "no publish-on-web"; stale task URLs no longer leak through after the config is removed. Legacy rows lack the "urls" key so the test still picks them up. 3. (Minor) New test: describe_deployment propagates non-404 (401/etc) from the disambiguation probe, mirroring the list_deployments test. 4. (Minor) New parametrized test for _status_color covering every DeploymentStatus enum value plus the empty-string fallback. --- src/zad_cli/api/client.py | 39 ++++++++++++---- tests/commands/test_deployment.py | 24 ++++++++++ tests/test_client.py | 77 +++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 10 deletions(-) create mode 100644 tests/commands/test_deployment.py diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index b600231..e9fae2f 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -8,6 +8,7 @@ from urllib.parse import urljoin import httpx +from pydantic import ValidationError from zad_cli.api.models import DeploymentDetail, DeploymentListResponse, TaskStatus @@ -42,6 +43,21 @@ def __init__(self, message: str, details: dict | None = None): _RETRYABLE_CODES = {429, 500, 502, 503, 504} +def _parse_v2_response(model_cls: type, payload: Any) -> dict: + """Validate a v2 response through pydantic and return it as a dict. + + Translates `pydantic.ValidationError` into `ZadApiError(502, ...)` so + schema drift (new enum values, missing fields) surfaces through the + same error path as HTTP errors. The decorator chain on CLI commands + catches `ZadApiError` and renders it cleanly; an uncaught + `ValidationError` would bypass that and produce a raw traceback. + """ + try: + return model_cls.model_validate(payload).model_dump(mode="json") + except ValidationError as e: + raise ZadApiError(502, f"Unexpected API response shape for {model_cls.__name__}: {e}") from e + + class ZadClient: """Synchronous HTTP client for the ZAD Operations Manager API.""" @@ -447,13 +463,12 @@ def list_deployments_v2(self, project: str) -> dict: deployments[]). Each deployment carries status, sync_revision, last_synced_at, errors, components, and urls. - The response is validated against `DeploymentListResponse` and then - re-serialized to a dict. Validation surfaces upstream schema drift - as a clear pydantic error instead of letting malformed data leak - downstream. + Validation surfaces upstream schema drift as a `ZadApiError(502)` + instead of letting `pydantic.ValidationError` escape past the + `@handle_api_errors` decorator and produce a raw traceback. """ response = self._request("GET", f"/v2/projects/{project}/deployments") - return DeploymentListResponse.model_validate(response.json()).model_dump(mode="json") + return _parse_v2_response(DeploymentListResponse, response.json()) def get_deployment_v2(self, project: str, deployment: str) -> dict: """Read a single deployment from the v2 read endpoint. @@ -461,7 +476,7 @@ def get_deployment_v2(self, project: str, deployment: str) -> dict: Returns the `DeploymentDetail` shape, validated through pydantic. """ response = self._request("GET", f"/v2/projects/{project}/deployments/{deployment}") - return DeploymentDetail.model_validate(response.json()).model_dump(mode="json") + return _parse_v2_response(DeploymentDetail, response.json()) # --- Project introspection --- @@ -676,11 +691,15 @@ def project_status(self, project: str) -> dict: if dep_name not in urls_by_deployment and dep_urls.get("urls"): urls_by_deployment[dep_name] = dep_urls["urls"] - # Enrich deployments with URLs. v2 is authoritative when present; - # the task probe is a best-effort fallback for legacy upstreams. + # Enrich deployments with URLs. v2 is authoritative when present + # (an empty dict from v2 means "no public URLs", not "no data"); + # the task probe is a fallback for legacy rows, which never carry + # the "urls" key. Distinguishing presence from emptiness avoids + # surfacing stale task URLs after publish-on-web is removed. for dep in deployments: - task_urls = urls_by_deployment.get(dep["deployment"], {}) - dep["urls"] = dep.get("urls", {}) or task_urls + if "urls" in dep: + continue + dep["urls"] = urls_by_deployment.get(dep["deployment"], {}) return { "project": project, diff --git a/tests/commands/test_deployment.py b/tests/commands/test_deployment.py new file mode 100644 index 0000000..c39f604 --- /dev/null +++ b/tests/commands/test_deployment.py @@ -0,0 +1,24 @@ +"""Unit tests for command-level helpers in commands/deployment.py.""" + +import pytest + +from zad_cli.commands.deployment import _status_color + + +@pytest.mark.parametrize( + "status,expected", + [ + ("Healthy", "green"), + ("Degraded", "red"), + ("Missing", "red"), + ("OutOfSync", "red"), + ("Suspended", "red"), + ("Progressing", "yellow"), + ("Pending", "yellow"), + ("Unavailable", "dim"), + ("Unknown", "dim"), + ("", "dim"), + ], +) +def test_status_color(status: str, expected: str) -> None: + assert _status_color(status) == expected diff --git a/tests/test_client.py b/tests/test_client.py index f259799..a0657e0 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -531,6 +531,83 @@ def test_project_status_v2_urls_win_over_stale_task_urls(client): assert result["deployments"][0]["urls"] == {"web": "https://current.example.com"} +@respx.mock +def test_project_status_preserves_v2_empty_urls(client): + """When v2 explicitly returns empty urls (no publish-on-web), don't surface stale task URLs.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response( + 200, + json={ + "project": "my-project", + "cluster": "odcn-test", + "deployments": [ + { + "name": "staging", + "project": "my-project", + "cluster": "odcn-test", + "namespace": "ns-staging", + "components": [{"reference": "worker", "image": "ghcr.io/org/worker:v1"}], + "urls": {}, + "status": "Healthy", + } + ], + }, + ) + ) + respx.get("https://api.example.com/subdomains").mock(return_value=httpx.Response(200, json={"items": []})) + respx.get("https://api.example.com/tasks").mock( + return_value=httpx.Response( + 200, + json={ + "tasks": [ + { + "status": "completed", + "result": {"urls": {"staging": {"urls": {"web": "https://stale.example.com"}}}}, + } + ] + }, + ) + ) + + result = client.project_status("my-project") + + assert result["deployments"][0]["urls"] == {} + + +@respx.mock +def test_describe_deployment_propagates_non_404_from_probe(client): + """If the disambiguation probe returns 401/403/500, propagate the real cause.""" + respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response(401, json={"detail": "Unauthorized"}) + ) + + with pytest.raises(ZadApiError) as exc_info: + client.describe_deployment("my-project", "staging") + + assert exc_info.value.status_code == 401 + + +@respx.mock +def test_v2_validation_error_becomes_502(client): + """An upstream response that fails pydantic validation surfaces as ZadApiError(502).""" + respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( + return_value=httpx.Response( + 200, + # Missing required `cluster` field; would crash without the wrapper. + json={"project": "my-project", "deployments": []}, + ) + ) + + with pytest.raises(ZadApiError) as exc_info: + client.list_deployments_v2("my-project") + + assert exc_info.value.status_code == 502 + assert "DeploymentListResponse" in str(exc_info.value) + + @respx.mock def test_list_deployments_uses_legacy_when_projects_endpoint_also_404s(client): """Very old upstream where /projects itself doesn't exist: assume project exists, fall back to legacy.""" From e7c6f0a41da8245de7a409758b20e8e51030276a Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 12:57:56 +0200 Subject: [PATCH 10/14] fix: address fifth review on PR #23 - New CLI integration tests for deployment describe rendering, using typer's CliRunner with a stubbed ZadClient. Covers Healthy (no Errors table) and Degraded (Errors table with category, message, and explanation footer) so future Rich-markup or key-name regressions get caught at CI. - Trim multi-paragraph docstrings on _parse_v2_response, list_deployments_v2, get_deployment_v2, list_deployments, describe_deployment, _project_exists per CLAUDE.md (one-line docstrings only). Reasoning lives in the PR description. - _status_color now uses a dict keyed on DeploymentStatus enum members instead of bare string literals; if upstream renames a status the type system surfaces the dependency. --- src/zad_cli/api/client.py | 53 ++--------------- src/zad_cli/commands/deployment.py | 21 ++++--- tests/commands/test_deployment.py | 93 +++++++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 56 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index e9fae2f..4359faa 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -44,14 +44,7 @@ def __init__(self, message: str, details: dict | None = None): def _parse_v2_response(model_cls: type, payload: Any) -> dict: - """Validate a v2 response through pydantic and return it as a dict. - - Translates `pydantic.ValidationError` into `ZadApiError(502, ...)` so - schema drift (new enum values, missing fields) surfaces through the - same error path as HTTP errors. The decorator chain on CLI commands - catches `ZadApiError` and renders it cleanly; an uncaught - `ValidationError` would bypass that and produce a raw traceback. - """ + """Validate a v2 response and re-emit as dict, translating ValidationError to ZadApiError(502).""" try: return model_cls.model_validate(payload).model_dump(mode="json") except ValidationError as e: @@ -457,24 +450,12 @@ def get_logs( # --- V2 deployment read endpoints --- def list_deployments_v2(self, project: str) -> dict: - """Read all deployments in a project from the v2 read endpoint. - - Returns the `DeploymentListResponse` shape (project, cluster, - deployments[]). Each deployment carries status, sync_revision, - last_synced_at, errors, components, and urls. - - Validation surfaces upstream schema drift as a `ZadApiError(502)` - instead of letting `pydantic.ValidationError` escape past the - `@handle_api_errors` decorator and produce a raw traceback. - """ + """Read all deployments in a project from the v2 read endpoint.""" response = self._request("GET", f"/v2/projects/{project}/deployments") return _parse_v2_response(DeploymentListResponse, response.json()) def get_deployment_v2(self, project: str, deployment: str) -> dict: - """Read a single deployment from the v2 read endpoint. - - Returns the `DeploymentDetail` shape, validated through pydantic. - """ + """Read a single deployment from the v2 read endpoint.""" response = self._request("GET", f"/v2/projects/{project}/deployments/{deployment}") return _parse_v2_response(DeploymentDetail, response.json()) @@ -489,13 +470,7 @@ def resolve_namespace(self, project: str, deployment: str) -> str: raise ZadApiError(404, f"Deployment '{deployment}' not found in project '{project}'") def list_deployments(self, project: str) -> list[dict]: - """List all deployments and their components in a project. - - Prefers the v2 read endpoint. Falls back to the logs+tasks fusion - only when the upstream Operations Manager predates the read endpoint. - Returns the legacy shape: deployment, project, namespace, - components (list of names), status. - """ + """List all deployments in a project; prefers v2, falls back to logs+tasks on old upstreams.""" try: data = self.list_deployments_v2(project) except ZadApiError as e: @@ -523,13 +498,7 @@ def list_deployments(self, project: str) -> list[dict]: return rows def describe_deployment(self, project: str, deployment: str) -> dict: - """Get detailed info about a deployment. - - Prefers the v2 read endpoint. Falls back to the logs+tasks fusion - only when the upstream Operations Manager predates the read endpoint. - Returns the legacy shape augmented with status, sync_revision, - last_synced_at, and errors when the v2 endpoint is available. - """ + """Get a single deployment's detail; prefers v2, falls back to logs+tasks on old upstreams.""" try: dep = self.get_deployment_v2(project, deployment) except ZadApiError as e: @@ -565,21 +534,11 @@ def describe_deployment(self, project: str, deployment: str) -> dict: } def _project_exists(self, project: str) -> bool: - """Probe whether a project exists on this upstream. - - Used to disambiguate 404s from list_deployments_v2 / get_deployment_v2. - list_projects works on every upstream version, so it is the - authoritative source. Only treats a 404 from list_projects itself as - "endpoint missing, fall back" — every other error (401/403/5xx) - propagates so the caller sees the real cause. - """ + """Disambiguation probe: True if /projects lists this project (or itself 404s on very old upstreams).""" try: projects = self.list_projects() except ZadApiError as err: if err.status_code == 404: - # /projects itself is missing on this upstream. Best effort - # is to assume the project exists so the legacy fallback - # gets a chance. return True raise return any(p.get("name") == project for p in projects) diff --git a/src/zad_cli/commands/deployment.py b/src/zad_cli/commands/deployment.py index 084720f..e19a992 100644 --- a/src/zad_cli/commands/deployment.py +++ b/src/zad_cli/commands/deployment.py @@ -6,7 +6,7 @@ import typer -from zad_cli.api.models import Component, UpsertDeploymentRequest +from zad_cli.api.models import Component, DeploymentStatus, UpsertDeploymentRequest from zad_cli.helpers import ( complete_deployment, confirm_action, @@ -56,15 +56,20 @@ def list_deployments(ctx: typer.Context) -> None: ) +_STATUS_COLORS: dict[str, str] = { + DeploymentStatus.HEALTHY: "green", + DeploymentStatus.DEGRADED: "red", + DeploymentStatus.MISSING: "red", + DeploymentStatus.OUT_OF_SYNC: "red", + DeploymentStatus.SUSPENDED: "red", + DeploymentStatus.PROGRESSING: "yellow", + DeploymentStatus.PENDING: "yellow", +} + + def _status_color(status: str) -> str: """Color for a DeploymentStatus enum value.""" - if status == "Healthy": - return "green" - if status in ("Degraded", "Missing", "OutOfSync", "Suspended"): - return "red" - if status in ("Progressing", "Pending"): - return "yellow" - return "dim" + return _STATUS_COLORS.get(status, "dim") @app.command() diff --git a/tests/commands/test_deployment.py b/tests/commands/test_deployment.py index c39f604..445a0db 100644 --- a/tests/commands/test_deployment.py +++ b/tests/commands/test_deployment.py @@ -1,7 +1,11 @@ -"""Unit tests for command-level helpers in commands/deployment.py.""" +"""Unit tests for command-level helpers and rendering in commands/deployment.py.""" + +from typing import Any import pytest +from typer.testing import CliRunner +from zad_cli.cli import app from zad_cli.commands.deployment import _status_color @@ -22,3 +26,90 @@ ) def test_status_color(status: str, expected: str) -> None: assert _status_color(status) == expected + + +def _stub_describe(monkeypatch: pytest.MonkeyPatch, payload: dict[str, Any]) -> None: + """Stub the client describe_deployment + the Settings auth so the command runs.""" + + class _StubClient: + def __init__(self, *_args, **_kwargs): + self.wait = True + self.verbose = False + + def describe_deployment(self, _project: str, _deployment: str) -> dict[str, Any]: + return payload + + def close(self) -> None: + pass + + monkeypatch.setenv("ZAD_API_KEY", "test-key") + monkeypatch.setenv("ZAD_PROJECT_ID", "my-project") + monkeypatch.setenv("ZAD_API_URL", "https://api.example.com") + monkeypatch.setattr("zad_cli.helpers.ZadClient", _StubClient, raising=False) + # The import inside _ensure_client uses a deferred import; patch that attribute too. + import zad_cli.api.client as client_module + + monkeypatch.setattr(client_module, "ZadClient", _StubClient) + + +def test_describe_renders_healthy_deployment(monkeypatch: pytest.MonkeyPatch) -> None: + _stub_describe( + monkeypatch, + { + "deployment": "staging", + "project": "my-project", + "namespace": "ns-staging", + "components": [{"name": "web", "image": "ghcr.io/org/web:v1", "k8s_deployment": ""}], + "urls": {"web": "https://staging.example.com"}, + "status": "Healthy", + "sync_revision": "abc123def456", + "last_synced_at": "2026-05-07T09:00:00Z", + "errors": [], + }, + ) + + runner = CliRunner() + result = runner.invoke(app, ["deployment", "describe", "staging"]) + + assert result.exit_code == 0, result.output + assert "staging" in result.output + assert "Healthy" in result.output + assert "abc123def456" in result.output + assert "Last sync attempt" in result.output + assert "https://staging.example.com" in result.output + # No errors table when healthy. + assert "Errors" not in result.output + + +def test_describe_renders_degraded_deployment_with_errors(monkeypatch: pytest.MonkeyPatch) -> None: + _stub_describe( + monkeypatch, + { + "deployment": "staging", + "project": "my-project", + "namespace": "ns-staging", + "components": [{"name": "web", "image": "ghcr.io/org/web:bad", "k8s_deployment": ""}], + "urls": {}, + "status": "Degraded", + "sync_revision": "deadbeefcafe", + "last_synced_at": "2026-05-07T08:00:00Z", + "errors": [ + { + "resource": "Pod/web-7c9d8f-xxxxx", + "message": "Back-off pulling image ghcr.io/org/web:bad", + "category": "ImagePull", + "explanation": "Container image cannot be pulled.", + } + ], + }, + ) + + runner = CliRunner() + result = runner.invoke(app, ["deployment", "describe", "staging"]) + + assert result.exit_code == 0, result.output + assert "Degraded" in result.output + assert "Errors" in result.output + assert "ImagePull" in result.output + assert "Back-off pulling image" in result.output + assert "Container image cannot be pulled." in result.output From 8121d2b4a02efebd422ef37877a98dd009e2829a Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 13:08:04 +0200 Subject: [PATCH 11/14] refactor: drop legacy logs+tasks fallback paths There is one production upstream and it has been upgraded to ship the v2 read endpoints, so the fallback chain no longer earns its keep. Removed: - _list_deployments_legacy and _describe_deployment_legacy (logs+tasks fusion to reconstruct deployment state) - _project_exists disambiguation probe and the layered 404 handling in list_deployments / describe_deployment - project_status's /tasks call to recover URLs (v2 supplies them directly on every deployment) - The optional `subdomain` plumbing and `k8s_deployment` column rendering, both vestigial after the legacy path is gone - All tests covering legacy fallback paths, probe behavior, and stale task-history URL handling resolve_namespace now uses get_deployment_v2 directly instead of walking list_deployments. describe_deployment, list_deployments, and project_status pass through v2 fields without `.get()` defaults: after pydantic validation the required fields are guaranteed present and the optional ones are explicitly None. Net change: ~150 lines of client code and ~250 lines of tests removed, no behavior change against the current upstream. --- src/zad_cli/api/client.py | 213 +++-------------- src/zad_cli/commands/deployment.py | 40 +--- tests/test_client.py | 367 +---------------------------- 3 files changed, 42 insertions(+), 578 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index 4359faa..9a5da76 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -463,203 +463,46 @@ def get_deployment_v2(self, project: str, deployment: str) -> dict: def resolve_namespace(self, project: str, deployment: str) -> str: """Resolve a deployment name to its Kubernetes namespace.""" - deployments = self.list_deployments(project) - for dep in deployments: - if dep["deployment"] == deployment: - return dep["namespace"] - raise ZadApiError(404, f"Deployment '{deployment}' not found in project '{project}'") + return self.get_deployment_v2(project, deployment)["namespace"] def list_deployments(self, project: str) -> list[dict]: - """List all deployments in a project; prefers v2, falls back to logs+tasks on old upstreams.""" - try: - data = self.list_deployments_v2(project) - except ZadApiError as e: - if e.status_code == 404: - if self._project_exists(project): - return self._list_deployments_legacy(project) - raise ZadApiError(404, f"Project '{project}' not found") from e - raise - - rows: list[dict] = [] - for dep in data.get("deployments", []): - rows.append( - { - "deployment": dep["name"], - "project": dep.get("project", project), - "namespace": dep.get("namespace", ""), - "components": [c["reference"] for c in dep.get("components", [])], - "status": dep.get("status", "Unknown"), - "urls": dep.get("urls", {}), - "sync_revision": dep.get("sync_revision"), - "last_synced_at": dep.get("last_synced_at"), - "errors": dep.get("errors", []), - } - ) - return rows + """List all deployments in a project.""" + data = self.list_deployments_v2(project) + return [ + { + "deployment": dep["name"], + "project": dep["project"], + "namespace": dep["namespace"], + "components": [c["reference"] for c in dep["components"]], + "status": dep["status"], + "urls": dep["urls"], + "sync_revision": dep["sync_revision"], + "last_synced_at": dep["last_synced_at"], + "errors": dep["errors"], + } + for dep in data["deployments"] + ] def describe_deployment(self, project: str, deployment: str) -> dict: - """Get a single deployment's detail; prefers v2, falls back to logs+tasks on old upstreams.""" - try: - dep = self.get_deployment_v2(project, deployment) - except ZadApiError as e: - if e.status_code == 404: - # The 404 means one of three things: deployment missing, - # project missing, or v2 endpoint not registered on this - # upstream. Probe list_deployments_v2 to separate the first - # two from the third; then probe list_projects to separate - # the first from the second. - try: - self.list_deployments_v2(project) - except ZadApiError as list_err: - if list_err.status_code == 404: - if self._project_exists(project): - return self._describe_deployment_legacy(project, deployment) - raise ZadApiError(404, f"Project '{project}' not found") from e - raise - raise ZadApiError(404, f"Deployment '{deployment}' not found in project '{project}'") from e - raise - + """Get a single deployment's detail.""" + dep = self.get_deployment_v2(project, deployment) return { "deployment": dep["name"], - "project": dep.get("project", project), - "namespace": dep.get("namespace", ""), - "components": [ - {"name": c["reference"], "image": c["image"], "k8s_deployment": ""} for c in dep.get("components", []) - ], - "urls": dep.get("urls", {}), - "status": dep.get("status", "Unknown"), - "sync_revision": dep.get("sync_revision"), - "last_synced_at": dep.get("last_synced_at"), - "errors": dep.get("errors", []), - } - - def _project_exists(self, project: str) -> bool: - """Disambiguation probe: True if /projects lists this project (or itself 404s on very old upstreams).""" - try: - projects = self.list_projects() - except ZadApiError as err: - if err.status_code == 404: - return True - raise - return any(p.get("name") == project for p in projects) - - # --- Legacy fallbacks (logs+tasks fusion) --- - - def _list_deployments_legacy(self, project: str) -> list[dict]: - response = self._request("GET", f"/logs/{project}", params={"limit": "0"}) - data = response.json() - - deployments: dict[str, dict] = {} - for entry in data.get("results", []): - dep_name = entry["deployment"] - if dep_name not in deployments: - deployments[dep_name] = { - "deployment": dep_name, - "project": entry.get("project", project), - "namespace": entry.get("namespace", ""), - "components": [], - "status": "Active", - } - deployments[dep_name]["components"].append(entry["component"]) - - task_response = self._request("GET", "/tasks", params={"project_name": project}) - tasks = task_response.json().get("tasks", []) - seen: set[str] = set() - for task in tasks: - dep_name = (task.get("result") or {}).get("deployment") or {} - dep_name = dep_name.get("name", "") if isinstance(dep_name, dict) else "" - if not dep_name or dep_name in seen or dep_name not in deployments: - continue - seen.add(dep_name) - if task.get("status") == "failed": - deployments[dep_name]["status"] = "Failed" - elif task.get("status") in ("pending", "running"): - deployments[dep_name]["status"] = "Deploying" - - return list(deployments.values()) - - def _describe_deployment_legacy(self, project: str, deployment: str) -> dict: - response = self._request("GET", f"/logs/{project}", params={"deployment": deployment, "limit": "0"}) - log_data = response.json() - - components = [] - namespace = "" - for entry in log_data.get("results", []): - namespace = entry.get("namespace", namespace) - components.append({"name": entry["component"], "k8s_deployment": entry.get("k8s_deployment", "")}) - - urls: dict[str, str] = {} - images: dict[str, str] = {} - task_response = self._request( - "GET", - "/tasks", - params={"project_name": project, "deployment_name": deployment, "status": "completed"}, - ) - tasks = task_response.json().get("tasks", []) - for task in tasks: - result = task.get("result") or {} - if not isinstance(result, dict): - continue - dep_info = result.get("deployment") or {} - if not isinstance(dep_info, dict): - continue - if dep_info.get("name") != deployment: - continue - urls_data = result.get("urls") or {} - dep_data = urls_data.get(deployment) or {} - dep_urls = dep_data.get("urls", {}) if isinstance(dep_data, dict) else {} - if dep_urls and not urls: - urls = dep_urls - for comp in dep_info.get("components", []): - ref = comp.get("reference", "") - if ref and ref not in images: - images[ref] = comp.get("image", "") - - for comp in components: - comp["image"] = images.get(comp["name"], "") - - return { - "deployment": deployment, - "project": project, - "namespace": namespace, - "components": components, - "urls": urls, + "project": dep["project"], + "namespace": dep["namespace"], + "components": [{"name": c["reference"], "image": c["image"]} for c in dep["components"]], + "urls": dep["urls"], + "status": dep["status"], + "sync_revision": dep["sync_revision"], + "last_synced_at": dep["last_synced_at"], + "errors": dep["errors"], } def project_status(self, project: str) -> dict: - """Get a project overview: deployments, components, subdomains.""" + """Get a project overview: deployments and subdomains.""" deployments = self.list_deployments(project) - - # Get subdomain info subdomain_response = self._request("GET", "/subdomains", params={"project_name": project}) subdomains = subdomain_response.json().get("items", []) - - # Get URLs from recent tasks - task_response = self._request("GET", "/tasks", params={"project_name": project}) - tasks = task_response.json().get("tasks", []) - urls_by_deployment: dict[str, dict] = {} - for task in tasks: - if task.get("status") != "completed": - continue - result = task.get("result") or {} - if not isinstance(result, dict): - continue - for dep_name, dep_urls in result.get("urls", {}).items(): - if not isinstance(dep_urls, dict): - continue - if dep_name not in urls_by_deployment and dep_urls.get("urls"): - urls_by_deployment[dep_name] = dep_urls["urls"] - - # Enrich deployments with URLs. v2 is authoritative when present - # (an empty dict from v2 means "no public URLs", not "no data"); - # the task probe is a fallback for legacy rows, which never carry - # the "urls" key. Distinguishing presence from emptiness avoids - # surfacing stale task URLs after publish-on-web is removed. - for dep in deployments: - if "urls" in dep: - continue - dep["urls"] = urls_by_deployment.get(dep["deployment"], {}) - return { "project": project, "deployments": deployments, diff --git a/src/zad_cli/commands/deployment.py b/src/zad_cli/commands/deployment.py index e19a992..52724a3 100644 --- a/src/zad_cli/commands/deployment.py +++ b/src/zad_cli/commands/deployment.py @@ -101,58 +101,42 @@ def describe( console.print(f"[bold]Project:[/bold] {result['project']}") console.print(f"[bold]Namespace:[/bold] {result['namespace']}") - status = result.get("status") - if status: - color = _status_color(status) - console.print(f"[bold]Status:[/bold] [{color}]{status}[/{color}]") - if result.get("sync_revision"): + color = _status_color(result["status"]) + console.print(f"[bold]Status:[/bold] [{color}]{result['status']}[/{color}]") + if result["sync_revision"]: console.print(f"[bold]Revision:[/bold] {result['sync_revision'][:12]}") - if result.get("last_synced_at"): - # Per upstream spec: this is the last sync *attempt*, not necessarily - # successful. Phrase the label so a Degraded deployment doesn't look - # like it last synced cleanly. + if result["last_synced_at"]: + # Upstream documents this as the last sync attempt regardless of + # outcome, so phrasing avoids implying a clean state. console.print(f"[bold]Last sync attempt:[/bold] {result['last_synced_at']}") - if result.get("urls"): + if result["urls"]: console.print("\n[bold]URLs:[/bold]") for comp_name, url in result["urls"].items(): console.print(f" {comp_name}: {url}") console.print() - has_images = any(comp.get("image") for comp in result["components"]) - has_k8s = any(comp.get("k8s_deployment") for comp in result["components"]) - table = Table(title="Components", show_header=True) table.add_column("Name", style="bold cyan") - if has_images: - table.add_column("Image") - if has_k8s: - table.add_column("K8s Deployment") - + table.add_column("Image") for comp in result["components"]: - row = [comp["name"]] - if has_images: - row.append(comp.get("image", "")) - if has_k8s: - row.append(comp.get("k8s_deployment", "")) - table.add_row(*row) - + table.add_row(comp["name"], comp["image"]) console.print(table) - errors = result.get("errors") or [] + errors = result["errors"] if errors: err_table = Table(title="Errors", show_header=True, header_style="bold red") err_table.add_column("Category", style="bold") err_table.add_column("Resource") err_table.add_column("Message") for err in errors: - err_table.add_row(err.get("category", ""), err.get("resource", ""), err.get("message", "")) + err_table.add_row(err["category"], err["resource"], err["message"]) console.print(err_table) seen_explanations: set[str] = set() for err in errors: - cat = err.get("category", "") + cat = err["category"] explanation = err.get("explanation") if explanation and cat not in seen_explanations: seen_explanations.add(cat) diff --git a/tests/test_client.py b/tests/test_client.py index a0657e0..ec7e914 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -146,104 +146,6 @@ def test_api_key_header(client): assert route.calls[0].request.headers["X-API-Key"] == "test-key" -@respx.mock -def test_describe_deployment_filters_tasks_server_side(client): - """Legacy fallback: when both v2 endpoints 404, describe must narrow /tasks server-side.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(200, json=[{"name": "my-project"}])) - respx.get("https://api.example.com/logs/my-project").mock( - return_value=httpx.Response( - 200, - json={ - "results": [ - {"component": "web", "deployment": "staging", "namespace": "ns-staging", "k8s_deployment": "web"}, - ] - }, - ) - ) - tasks_route = respx.get("https://api.example.com/tasks").mock( - return_value=httpx.Response( - 200, - json={ - "tasks": [ - { - "status": "completed", - "result": { - "deployment": { - "name": "staging", - "components": [{"reference": "web", "image": "ghcr.io/org/web:v1"}], - }, - "urls": {"staging": {"urls": {"web": "https://staging.example.com"}}}, - }, - }, - ] - }, - ) - ) - - result = client.describe_deployment("my-project", "staging") - - assert tasks_route.called - params = tasks_route.calls[0].request.url.params - assert params["project_name"] == "my-project" - assert params["deployment_name"] == "staging" - assert params["status"] == "completed" - - assert result["urls"] == {"web": "https://staging.example.com"} - assert result["components"][0]["image"] == "ghcr.io/org/web:v1" - - -@respx.mock -def test_describe_deployment_ignores_tasks_with_mismatched_name(client): - """Legacy fallback: if the server filter ever leaks a mismatched task, the client guard must drop it.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(200, json=[{"name": "my-project"}])) - respx.get("https://api.example.com/logs/my-project").mock( - return_value=httpx.Response( - 200, - json={ - "results": [ - {"component": "web", "deployment": "staging", "namespace": "ns-staging", "k8s_deployment": "web"}, - ] - }, - ) - ) - respx.get("https://api.example.com/tasks").mock( - return_value=httpx.Response( - 200, - json={ - "tasks": [ - { - "status": "completed", - "result": { - "deployment": { - "name": "staging-prefix-leak", - "components": [{"reference": "web", "image": "ghcr.io/org/web:wrong"}], - }, - "urls": {"staging-prefix-leak": {"urls": {"web": "https://wrong.example.com"}}}, - }, - }, - ] - }, - ) - ) - - result = client.describe_deployment("my-project", "staging") - - assert result["urls"] == {} - assert result["components"][0]["image"] == "" - - @respx.mock def test_describe_deployment_uses_v2_endpoint(client): """describe_deployment prefers the v2 read endpoint when available.""" @@ -366,228 +268,16 @@ def test_list_deployments_uses_v2_endpoint(client): @respx.mock -def test_describe_deployment_propagates_404_when_v2_endpoint_exists(client): - """When the v2 list endpoint works but the get endpoint 404s, the deployment is genuinely missing.""" +def test_describe_deployment_propagates_404(client): + """A 404 from the v2 endpoint surfaces directly.""" respx.get("https://api.example.com/v2/projects/my-project/deployments/missing").mock( return_value=httpx.Response(404, json={"detail": "not found"}) ) - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response( - 200, - json={"project": "my-project", "cluster": "odcn-test", "deployments": []}, - ) - ) with pytest.raises(ZadApiError) as exc_info: client.describe_deployment("my-project", "missing") assert exc_info.value.status_code == 404 - assert "missing" in str(exc_info.value) - - -@respx.mock -def test_describe_deployment_falls_back_on_old_upstream(client): - """When both v2 endpoints 404 and the project exists, the upstream lacks read endpoints; use legacy.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(200, json=[{"name": "my-project"}])) - respx.get("https://api.example.com/logs/my-project").mock( - return_value=httpx.Response( - 200, - json={"results": [{"component": "web", "deployment": "staging", "namespace": "ns-staging"}]}, - ) - ) - respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) - - result = client.describe_deployment("my-project", "staging") - - assert result["deployment"] == "staging" - assert result["namespace"] == "ns-staging" - - -@respx.mock -def test_describe_deployment_propagates_404_when_project_missing(client): - """v2 endpoints 404 and the project is not in list_projects: surface 'Project not found'.""" - respx.get("https://api.example.com/v2/projects/missing-project/deployments/staging").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/v2/projects/missing-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock( - return_value=httpx.Response(200, json=[{"name": "other-project"}]) - ) - - with pytest.raises(ZadApiError) as exc_info: - client.describe_deployment("missing-project", "staging") - - assert exc_info.value.status_code == 404 - assert "missing-project" in str(exc_info.value) - - -@respx.mock -def test_list_deployments_falls_back_on_old_upstream(client): - """v2 endpoint 404s but the project exists: upstream is old, use legacy fusion.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock( - return_value=httpx.Response(200, json=[{"name": "my-project"}, {"name": "other"}]) - ) - respx.get("https://api.example.com/logs/my-project").mock( - return_value=httpx.Response( - 200, - json={ - "results": [ - {"component": "web", "deployment": "staging", "namespace": "ns-staging"}, - ] - }, - ) - ) - respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) - - rows = client.list_deployments("my-project") - - assert len(rows) == 1 - assert rows[0]["deployment"] == "staging" - assert rows[0]["status"] == "Active" - - -@respx.mock -def test_project_status_preserves_v2_urls_when_no_recent_tasks(client): - """project_status must not clobber v2-supplied URLs with empty dicts when tasks have nothing to add.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response( - 200, - json={ - "project": "my-project", - "cluster": "odcn-test", - "deployments": [ - { - "name": "staging", - "project": "my-project", - "cluster": "odcn-test", - "namespace": "ns-staging", - "components": [{"reference": "web", "image": "ghcr.io/org/web:v1"}], - "urls": {"web": "https://staging.example.com"}, - "status": "Healthy", - } - ], - }, - ) - ) - respx.get("https://api.example.com/subdomains").mock(return_value=httpx.Response(200, json={"items": []})) - respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) - - result = client.project_status("my-project") - - assert result["deployments"][0]["urls"] == {"web": "https://staging.example.com"} - - -@respx.mock -def test_project_status_v2_urls_win_over_stale_task_urls(client): - """When both v2 and task history supply URLs, v2 is authoritative — stale task URLs must not win.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response( - 200, - json={ - "project": "my-project", - "cluster": "odcn-test", - "deployments": [ - { - "name": "staging", - "project": "my-project", - "cluster": "odcn-test", - "namespace": "ns-staging", - "components": [{"reference": "web", "image": "ghcr.io/org/web:v2"}], - "urls": {"web": "https://current.example.com"}, - "status": "Healthy", - } - ], - }, - ) - ) - respx.get("https://api.example.com/subdomains").mock(return_value=httpx.Response(200, json={"items": []})) - respx.get("https://api.example.com/tasks").mock( - return_value=httpx.Response( - 200, - json={ - "tasks": [ - { - "status": "completed", - "result": {"urls": {"staging": {"urls": {"web": "https://stale.example.com"}}}}, - } - ] - }, - ) - ) - - result = client.project_status("my-project") - - assert result["deployments"][0]["urls"] == {"web": "https://current.example.com"} - - -@respx.mock -def test_project_status_preserves_v2_empty_urls(client): - """When v2 explicitly returns empty urls (no publish-on-web), don't surface stale task URLs.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response( - 200, - json={ - "project": "my-project", - "cluster": "odcn-test", - "deployments": [ - { - "name": "staging", - "project": "my-project", - "cluster": "odcn-test", - "namespace": "ns-staging", - "components": [{"reference": "worker", "image": "ghcr.io/org/worker:v1"}], - "urls": {}, - "status": "Healthy", - } - ], - }, - ) - ) - respx.get("https://api.example.com/subdomains").mock(return_value=httpx.Response(200, json={"items": []})) - respx.get("https://api.example.com/tasks").mock( - return_value=httpx.Response( - 200, - json={ - "tasks": [ - { - "status": "completed", - "result": {"urls": {"staging": {"urls": {"web": "https://stale.example.com"}}}}, - } - ] - }, - ) - ) - - result = client.project_status("my-project") - - assert result["deployments"][0]["urls"] == {} - - -@respx.mock -def test_describe_deployment_propagates_non_404_from_probe(client): - """If the disambiguation probe returns 401/403/500, propagate the real cause.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments/staging").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(401, json={"detail": "Unauthorized"}) - ) - - with pytest.raises(ZadApiError) as exc_info: - client.describe_deployment("my-project", "staging") - - assert exc_info.value.status_code == 401 @respx.mock @@ -606,56 +296,3 @@ def test_v2_validation_error_becomes_502(client): assert exc_info.value.status_code == 502 assert "DeploymentListResponse" in str(exc_info.value) - - -@respx.mock -def test_list_deployments_uses_legacy_when_projects_endpoint_also_404s(client): - """Very old upstream where /projects itself doesn't exist: assume project exists, fall back to legacy.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock(return_value=httpx.Response(404, json={"detail": "not found"})) - respx.get("https://api.example.com/logs/my-project").mock( - return_value=httpx.Response( - 200, json={"results": [{"component": "web", "deployment": "staging", "namespace": "ns-staging"}]} - ) - ) - respx.get("https://api.example.com/tasks").mock(return_value=httpx.Response(200, json={"tasks": []})) - - rows = client.list_deployments("my-project") - - assert len(rows) == 1 - assert rows[0]["deployment"] == "staging" - - -@respx.mock -def test_list_deployments_propagates_non_404_from_probe(client): - """If list_projects returns 401/403/500, propagate the real error rather than masking it via legacy fallback.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock( - return_value=httpx.Response(401, json={"detail": "Unauthorized"}) - ) - - with pytest.raises(ZadApiError) as exc_info: - client.list_deployments("my-project") - - assert exc_info.value.status_code == 401 - - -@respx.mock -def test_list_deployments_propagates_404_when_project_missing(client): - """v2 endpoint 404s and the project is not in list_projects: raise 404, don't silently fall back.""" - respx.get("https://api.example.com/v2/projects/my-project/deployments").mock( - return_value=httpx.Response(404, json={"detail": "not found"}) - ) - respx.get("https://api.example.com/projects").mock( - return_value=httpx.Response(200, json=[{"name": "other-project"}]) - ) - - with pytest.raises(ZadApiError) as exc_info: - client.list_deployments("my-project") - - assert exc_info.value.status_code == 404 - assert "my-project" in str(exc_info.value) From 2095dc7b1ad43252ad62f25c4c68ad69442b3b87 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 13:16:50 +0200 Subject: [PATCH 12/14] fix: coerce unknown enum values instead of failing validation Closed enums were brittle: an additive upstream change (a new ErrorCategory or DeploymentStatus value) would make pydantic reject the entire DeploymentListResponse, taking the whole `zad deployment list` for a project down until the CLI is updated. Upstream's own `Unknown` catch-all signals they expect callers to handle unknown values gracefully, not hard-fail. - StatusError now coerces unknown category strings to UNKNOWN via a before-validator. - DeploymentDetail does the same for status. - Tests cover both coercion paths plus the known-value passthrough. - Drop vestigial `k8s_deployment` keys from the describe rendering test fixtures (the column was removed when v2 became the only source). --- src/zad_cli/api/models.py | 24 ++++++++++++++++++++++ tests/commands/test_deployment.py | 4 ++-- tests/test_models.py | 34 ++++++++++++++++++++++++++++++- 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/zad_cli/api/models.py b/src/zad_cli/api/models.py index 0773163..57de37a 100644 --- a/src/zad_cli/api/models.py +++ b/src/zad_cli/api/models.py @@ -185,6 +185,20 @@ class ErrorCategory(StrEnum): UNKNOWN = "Unknown" +def _coerce_unknown_category(v: object) -> object: + """Map an unknown ErrorCategory string to UNKNOWN so additive upstream enum changes don't break clients.""" + if isinstance(v, str) and v not in ErrorCategory._value2member_map_: + return ErrorCategory.UNKNOWN + return v + + +def _coerce_unknown_status(v: object) -> object: + """Same pattern for DeploymentStatus: unknown values degrade to UNKNOWN rather than rejecting the whole payload.""" + if isinstance(v, str) and v not in DeploymentStatus._value2member_map_: + return DeploymentStatus.UNKNOWN + return v + + class StatusError(BaseModel): """A single cluster-side error or warning surfaced on a deployment.""" @@ -194,6 +208,11 @@ class StatusError(BaseModel): explanation: str | None = None timestamp: str | None = None + @field_validator("category", mode="before") + @classmethod + def _coerce_category(cls, v: object) -> object: + return _coerce_unknown_category(v) + class DeploymentComponentDetail(BaseModel): """Component within a deployment as returned by the v2 read endpoints.""" @@ -217,6 +236,11 @@ class DeploymentDetail(BaseModel): last_synced_at: str | None = None errors: list[StatusError] = [] + @field_validator("status", mode="before") + @classmethod + def _coerce_status(cls, v: object) -> object: + return _coerce_unknown_status(v) + class DeploymentListResponse(BaseModel): """Response from GET /v2/projects/{p}/deployments.""" diff --git a/tests/commands/test_deployment.py b/tests/commands/test_deployment.py index 445a0db..f3d872a 100644 --- a/tests/commands/test_deployment.py +++ b/tests/commands/test_deployment.py @@ -59,7 +59,7 @@ def test_describe_renders_healthy_deployment(monkeypatch: pytest.MonkeyPatch) -> "deployment": "staging", "project": "my-project", "namespace": "ns-staging", - "components": [{"name": "web", "image": "ghcr.io/org/web:v1", "k8s_deployment": ""}], + "components": [{"name": "web", "image": "ghcr.io/org/web:v1"}], "urls": {"web": "https://staging.example.com"}, "status": "Healthy", "sync_revision": "abc123def456", @@ -88,7 +88,7 @@ def test_describe_renders_degraded_deployment_with_errors(monkeypatch: pytest.Mo "deployment": "staging", "project": "my-project", "namespace": "ns-staging", - "components": [{"name": "web", "image": "ghcr.io/org/web:bad", "k8s_deployment": ""}], + "components": [{"name": "web", "image": "ghcr.io/org/web:bad"}], "urls": {}, "status": "Degraded", "sync_revision": "deadbeefcafe", diff --git a/tests/test_models.py b/tests/test_models.py index 7189557..f39fcfe 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -3,7 +3,14 @@ import pytest from pydantic import ValidationError -from zad_cli.api.models import Component, UpsertDeploymentRequest +from zad_cli.api.models import ( + Component, + DeploymentDetail, + DeploymentStatus, + ErrorCategory, + StatusError, + UpsertDeploymentRequest, +) def test_valid_component(): @@ -66,3 +73,28 @@ def test_invalid_deployment_name(): deployment_name="bad name!", components=[Component(name="web", image="test")], ) + + +def test_status_error_coerces_unknown_category(): + """An ErrorCategory value not yet in our enum degrades to UNKNOWN, not a validation error.""" + err = StatusError.model_validate({"resource": "Pod/foo", "message": "boom", "category": "ResourceQuotaExceeded"}) + assert err.category == ErrorCategory.UNKNOWN + + +def test_status_error_keeps_known_category(): + err = StatusError.model_validate({"resource": "Pod/foo", "message": "boom", "category": "ImagePull"}) + assert err.category == ErrorCategory.IMAGE_PULL + + +def test_deployment_detail_coerces_unknown_status(): + """An unknown DeploymentStatus value degrades to UNKNOWN, keeping list_deployments resilient.""" + detail = DeploymentDetail.model_validate( + { + "name": "staging", + "project": "p", + "cluster": "c", + "namespace": "ns", + "status": "Reconciling", + } + ) + assert detail.status == DeploymentStatus.UNKNOWN From 402def1beadfbfe74846e285e2a548994b80c471 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 13:22:00 +0200 Subject: [PATCH 13/14] fix: address final review on PR #23 - Restore `k8s_deployment` as a tombstone field in describe_deployment's component dicts. The v2 endpoint doesn't expose this field, but removing it from the public describe shape would break consumers reading `comp["k8s_deployment"]` from the JSON output. Per the backwards-compat policy, return-shape removals require a deprecation cycle; an empty-string tombstone is the additive-only path. - Replace the private `_value2member_map_` access in the enum coercion validators with a public set comprehension `{e.value for e in Enum}`. - Use a realistic 40-char SHA in the describe rendering test so the `[:12]` truncation is actually exercised. --- src/zad_cli/api/client.py | 8 +++++++- src/zad_cli/api/models.py | 4 ++-- tests/commands/test_deployment.py | 4 +++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/zad_cli/api/client.py b/src/zad_cli/api/client.py index 9a5da76..57ef9c3 100644 --- a/src/zad_cli/api/client.py +++ b/src/zad_cli/api/client.py @@ -490,7 +490,13 @@ def describe_deployment(self, project: str, deployment: str) -> dict: "deployment": dep["name"], "project": dep["project"], "namespace": dep["namespace"], - "components": [{"name": c["reference"], "image": c["image"]} for c in dep["components"]], + "components": [ + # k8s_deployment is a tombstone for backwards compatibility: + # the v2 endpoint doesn't expose it, but consumers of the + # legacy describe shape may still read the key. + {"name": c["reference"], "image": c["image"], "k8s_deployment": ""} + for c in dep["components"] + ], "urls": dep["urls"], "status": dep["status"], "sync_revision": dep["sync_revision"], diff --git a/src/zad_cli/api/models.py b/src/zad_cli/api/models.py index 57de37a..6e7283f 100644 --- a/src/zad_cli/api/models.py +++ b/src/zad_cli/api/models.py @@ -187,14 +187,14 @@ class ErrorCategory(StrEnum): def _coerce_unknown_category(v: object) -> object: """Map an unknown ErrorCategory string to UNKNOWN so additive upstream enum changes don't break clients.""" - if isinstance(v, str) and v not in ErrorCategory._value2member_map_: + if isinstance(v, str) and v not in {e.value for e in ErrorCategory}: return ErrorCategory.UNKNOWN return v def _coerce_unknown_status(v: object) -> object: """Same pattern for DeploymentStatus: unknown values degrade to UNKNOWN rather than rejecting the whole payload.""" - if isinstance(v, str) and v not in DeploymentStatus._value2member_map_: + if isinstance(v, str) and v not in {e.value for e in DeploymentStatus}: return DeploymentStatus.UNKNOWN return v diff --git a/tests/commands/test_deployment.py b/tests/commands/test_deployment.py index f3d872a..2ab3b6e 100644 --- a/tests/commands/test_deployment.py +++ b/tests/commands/test_deployment.py @@ -62,7 +62,7 @@ def test_describe_renders_healthy_deployment(monkeypatch: pytest.MonkeyPatch) -> "components": [{"name": "web", "image": "ghcr.io/org/web:v1"}], "urls": {"web": "https://staging.example.com"}, "status": "Healthy", - "sync_revision": "abc123def456", + "sync_revision": "abc123def456" + "0" * 28, "last_synced_at": "2026-05-07T09:00:00Z", "errors": [], }, @@ -74,7 +74,9 @@ def test_describe_renders_healthy_deployment(monkeypatch: pytest.MonkeyPatch) -> assert result.exit_code == 0, result.output assert "staging" in result.output assert "Healthy" in result.output + # Truncated to the first 12 chars; the trailing zero-padding must not appear. assert "abc123def456" in result.output + assert "abc123def4560" not in result.output assert "Last sync attempt" in result.output assert "https://staging.example.com" in result.output # No errors table when healthy. From 174df44c0b44ad82405d01b59eb3b563a22a4ab5 Mon Sep 17 00:00:00 2001 From: Anne Schuth Date: Thu, 7 May 2026 13:28:31 +0200 Subject: [PATCH 14/14] fix: tighten _STATUS_COLORS type and exercise SHA truncation - _STATUS_COLORS is now annotated dict[DeploymentStatus, str], so type checkers flag a new enum value missing from the mapping instead of it silently falling through to "dim" at runtime. - The Degraded fixture's sync_revision was exactly 12 characters, so the [:12] truncation was a no-op there. Pad to 40 chars to match the Healthy fixture and actually verify truncation. --- src/zad_cli/commands/deployment.py | 2 +- tests/commands/test_deployment.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/zad_cli/commands/deployment.py b/src/zad_cli/commands/deployment.py index 52724a3..a74f4ae 100644 --- a/src/zad_cli/commands/deployment.py +++ b/src/zad_cli/commands/deployment.py @@ -56,7 +56,7 @@ def list_deployments(ctx: typer.Context) -> None: ) -_STATUS_COLORS: dict[str, str] = { +_STATUS_COLORS: dict[DeploymentStatus, str] = { DeploymentStatus.HEALTHY: "green", DeploymentStatus.DEGRADED: "red", DeploymentStatus.MISSING: "red", diff --git a/tests/commands/test_deployment.py b/tests/commands/test_deployment.py index 2ab3b6e..a31c224 100644 --- a/tests/commands/test_deployment.py +++ b/tests/commands/test_deployment.py @@ -93,7 +93,7 @@ def test_describe_renders_degraded_deployment_with_errors(monkeypatch: pytest.Mo "components": [{"name": "web", "image": "ghcr.io/org/web:bad"}], "urls": {}, "status": "Degraded", - "sync_revision": "deadbeefcafe", + "sync_revision": "deadbeefcafe" + "0" * 28, "last_synced_at": "2026-05-07T08:00:00Z", "errors": [ {