Skip to content

Commit 99f258f

Browse files
minitrigaclaude
andcommitted
Address minor review issues: module-level helper, consistent error messages, collection=False test, spec renames
- Move is_transport_failure to module level as _is_transport_failure - Replace f"Error: {detail}" 404 messages with consistent "No schema/collection named..." format - Remove now-unused contextlib.suppress import - Add test_collection_false_downloads_schema covering the defensive bool|None path - Update spec.md, plan.md, tasks.md: rename command references from download to get Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 3c40973 commit 99f258f

5 files changed

Lines changed: 48 additions & 25 deletions

File tree

dev/specs/001-marketplace-api-update/plan.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
## Summary
77

8-
Migrate `infrahubctl marketplace download` to the public REST API at `marketplace.infrahub.app`, add auto-detection of schema-vs-collection identifiers, keep the `--version` pinning flag for schemas, and retain `--output-dir` (default `./schemas`). The REST migration, `--version`, and `--output-dir` pieces have already shipped in the current branch's `Marketplace` commit; the remaining deltas are auto-detection, the four-class error taxonomy, and the documented precedence rule for namespace/name collisions.
8+
Migrate `infrahubctl marketplace get` to the public REST API at `marketplace.infrahub.app`, add auto-detection of schema-vs-collection identifiers, keep the `--version` pinning flag for schemas, and retain `--output-dir` (default `./schemas`). The REST migration, `--version`, and `--output-dir` pieces have already shipped in the current branch's `Marketplace` commit; the remaining deltas are auto-detection, the four-class error taxonomy, and the documented precedence rule for namespace/name collisions.
99

1010
## Technical Context
1111

dev/specs/001-marketplace-api-update/spec.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,22 @@
77

88
## Overview
99

10-
The public Infrahub Marketplace at `marketplace.infrahub.app` is now live and exposes a REST API for distributing schemas and schema collections. The existing `infrahubctl marketplace download` command was designed against an earlier GraphQL-based interface and must be realigned with the new REST contract. Alongside that migration, users need a simpler, less error-prone download experience: one identifier argument that Works For Schemas And Collections alike, an optional pinned version, and predictable default output placement.
10+
The public Infrahub Marketplace at `marketplace.infrahub.app` is now live and exposes a REST API for distributing schemas and schema collections. The existing `infrahubctl marketplace get` command was designed against an earlier GraphQL-based interface and must be realigned with the new REST contract. Alongside that migration, users need a simpler, less error-prone download experience: one identifier argument that Works For Schemas And Collections alike, an optional pinned version, and predictable default output placement.
1111

1212
## User Scenarios & Testing *(mandatory)*
1313

1414
### User Story 1 - Download any published item by identifier (Priority: P1)
1515

16-
As a platform engineer bootstrapping a new Infrahub environment, I want to run a single `infrahubctl marketplace download <namespace>/<name>` command and have the tool figure out whether I'm asking for a single schema or a full collection so that I don't need to inspect the marketplace UI or remember which flag to pass.
16+
As a platform engineer bootstrapping a new Infrahub environment, I want to run a single `infrahubctl marketplace get <namespace>/<name>` command and have the tool figure out whether I'm asking for a single schema or a full collection so that I don't need to inspect the marketplace UI or remember which flag to pass.
1717

1818
**Why this priority**: This is the primary workflow for every marketplace user. Without automatic detection, every first-time user who types the command will either guess wrong or abandon the command line and revisit the web UI, undermining the value of the CLI.
1919

2020
**Independent Test**: Publish one schema and one collection to the marketplace (or mock the API), then run the download command twice — once against each identifier — without passing any type-hint flag. Both invocations succeed, write the correct files, and print the correct item type.
2121

2222
**Acceptance Scenarios**:
2323

24-
1. **Given** the identifier `acme/network-base` refers to a single schema on the marketplace, **When** the user runs `infrahubctl marketplace download acme/network-base`, **Then** the CLI downloads the schema file to the default destination and reports it was downloaded as a schema.
25-
2. **Given** the identifier `acme/starter-pack` refers to a collection on the marketplace, **When** the user runs `infrahubctl marketplace download acme/starter-pack`, **Then** the CLI downloads every schema in the collection to the default destination and reports the collection totals.
24+
1. **Given** the identifier `acme/network-base` refers to a single schema on the marketplace, **When** the user runs `infrahubctl marketplace get acme/network-base`, **Then** the CLI downloads the schema file to the default destination and reports it was downloaded as a schema.
25+
2. **Given** the identifier `acme/starter-pack` refers to a collection on the marketplace, **When** the user runs `infrahubctl marketplace get acme/starter-pack`, **Then** the CLI downloads every schema in the collection to the default destination and reports the collection totals.
2626
3. **Given** the identifier does not match any published schema or collection, **When** the user runs the download command, **Then** the CLI fails with a clear message naming the identifier and the marketplace that was queried, and exits non-zero.
2727

2828
---
@@ -37,7 +37,7 @@ As a configuration author integrating a schema into a production pipeline, I wan
3737

3838
**Acceptance Scenarios**:
3939

40-
1. **Given** schema `acme/network-base` has published versions `0.9.0` and `1.2.0`, **When** the user runs `infrahubctl marketplace download acme/network-base --version 0.9.0`, **Then** the CLI writes the `0.9.0` payload to disk and reports that version in its success output.
40+
1. **Given** schema `acme/network-base` has published versions `0.9.0` and `1.2.0`, **When** the user runs `infrahubctl marketplace get acme/network-base --version 0.9.0`, **Then** the CLI writes the `0.9.0` payload to disk and reports that version in its success output.
4141
2. **Given** the user passes `--version` with a value that has not been published for the schema, **When** the command runs, **Then** the CLI fails with a message that distinguishes "version not found" from "schema not found" and exits non-zero.
4242
3. **Given** the target identifier resolves to a collection, **When** the user also passes `--version`, **Then** the CLI warns that `--version` has no effect for collections and proceeds with the collection download.
4343

@@ -72,7 +72,7 @@ As a repository owner whose project uses a non-standard layout, I want to direct
7272

7373
### Functional Requirements
7474

75-
- **FR-001**: The `infrahubctl marketplace download` command MUST communicate with the marketplace exclusively over its public REST API; no GraphQL usage for marketplace operations is permitted.
75+
- **FR-001**: The `infrahubctl marketplace get` command MUST communicate with the marketplace exclusively over its public REST API; no GraphQL usage for marketplace operations is permitted.
7676
- **FR-002**: The command MUST accept a single positional identifier in `namespace/name` form and reject any other shape with a usage error before making network calls.
7777
- **FR-003**: When no type-hint flag is passed, the command MUST automatically determine whether the identifier refers to a schema or a collection and download accordingly, reporting the resolved type in its output.
7878
- **FR-004**: The command MUST expose a `--version` option that, when provided, pins the download to that specific published semver for a schema.

dev/specs/001-marketplace-api-update/tasks.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ Single project: repository root contains `infrahub_sdk/` and `tests/`. All paths
4848

4949
## Phase 3: User Story 1 — Auto-detect schema vs. collection (Priority: P1) 🎯 MVP
5050

51-
**Goal**: A single `infrahubctl marketplace download <namespace>/<name>` command resolves to the correct item type without the user passing `--collection`, and prints the resolved type in success output.
51+
**Goal**: A single `infrahubctl marketplace get <namespace>/<name>` command resolves to the correct item type without the user passing `--collection`, and prints the resolved type in success output.
5252

5353
**Independent Test**: Mock one schema endpoint and one collection endpoint (as in `tests/unit/ctl/test_marketplace_app.py`). Run the download twice with no `--collection` flag — once against each identifier. Both succeed, files land at the expected paths, and the output names the resolved type.
5454

@@ -70,7 +70,7 @@ Single project: repository root contains `infrahub_sdk/` and `tests/`. All paths
7070
- [ ] T016 [US1] Route the existing 404 handling in `_download_schema` and `_download_collection` in `infrahub_sdk/ctl/marketplace.py` through `_fail("not-found", ...)`.
7171
- [ ] T017 [US1] Add network-error handling (`httpx.ConnectError`, `httpx.TimeoutException`, 5xx) wrapping the `async with httpx.AsyncClient...` block in `infrahub_sdk/ctl/marketplace.py`'s `download()` and route through `_fail("network", ...)`.
7272

73-
**Checkpoint**: Tests T006–T011 all pass. Running `infrahubctl marketplace download acme/starter-pack` (collection) and `infrahubctl marketplace download acme/network-base` (schema) both succeed without `--collection`, and the output names the resolved type. MVP scope.
73+
**Checkpoint**: Tests T006–T011 all pass. Running `infrahubctl marketplace get acme/starter-pack` (collection) and `infrahubctl marketplace get acme/network-base` (schema) both succeed without `--collection`, and the output names the resolved type. MVP scope.
7474

7575
---
7676

@@ -90,7 +90,7 @@ Single project: repository root contains `infrahub_sdk/` and `tests/`. All paths
9090
- [ ] T020 [US2] In `infrahub_sdk/ctl/marketplace.py` `_download_schema`, when `version` is provided and the versioned request returns 404, first retry an unversioned HEAD/GET against the same identifier to decide between "schema not found" and "version not found"; route through `_fail` with the appropriate class.
9191
- [ ] T021 [US2] Confirm that the existing "`--version` is ignored when downloading a collection" warning (currently at `marketplace.py:146-147`) still fires on the auto-detect path — i.e. when the user passed `--version` and the detected type is `collection`, the warning is printed before falling through to `_download_collection`. Add/adjust wiring in `download()` in `infrahub_sdk/ctl/marketplace.py` as needed.
9292

93-
**Checkpoint**: US1 still passes; T018 and T019 pass; `infrahubctl marketplace download acme/network-base --version 9.9.9` prints the version-not-found message and exits 1.
93+
**Checkpoint**: US1 still passes; T018 and T019 pass; `infrahubctl marketplace get acme/network-base --version 9.9.9` prints the version-not-found message and exits 1.
9494

9595
---
9696

infrahub_sdk/ctl/marketplace.py

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import asyncio
44
import sys
5-
from contextlib import suppress
65
from enum import Enum
76
from pathlib import Path
87
from typing import Any, Literal, NoReturn
@@ -70,6 +69,12 @@ def _collection_url(base_url: str, namespace: str, name: str) -> str:
7069
return f"{base_url}/api/v1/collections/{namespace}/{name}/download"
7170

7271

72+
def _is_transport_failure(r: object) -> bool:
73+
if isinstance(r, Exception):
74+
return True
75+
return isinstance(r, httpx.Response) and r.status_code >= 500
76+
77+
7378
def _mkdir_or_fail(path: Path) -> None:
7479
try:
7580
path.mkdir(parents=True, exist_ok=True)
@@ -123,12 +128,7 @@ async def _detect_item_type(
123128
if isinstance(collection_resp, httpx.Response) and collection_resp.status_code == 200:
124129
return "collection", collection_resp
125130

126-
def is_transport_failure(r: object) -> bool:
127-
if isinstance(r, Exception):
128-
return True
129-
return isinstance(r, httpx.Response) and r.status_code >= 500
130-
131-
if is_transport_failure(schema_resp) or is_transport_failure(collection_resp):
131+
if _is_transport_failure(schema_resp) or _is_transport_failure(collection_resp):
132132
_fail(
133133
_ErrorClass.NETWORK,
134134
f"Could not reach marketplace at {base_url}. Check your connection or --marketplace-url.",
@@ -172,10 +172,10 @@ async def _download_schema(
172172
f"Schema '{namespace}/{name}' has no published version '{version}'. "
173173
"Run without --version for the latest.",
174174
)
175-
detail = "not found"
176-
with suppress(Exception):
177-
detail = resp.json().get("detail", detail)
178-
_fail(_ErrorClass.NOT_FOUND, f"Error: {detail}")
175+
_fail(
176+
_ErrorClass.NOT_FOUND,
177+
f"No schema named '{namespace}/{name}' found on {_host_from(base_url)}.",
178+
)
179179
resp.raise_for_status()
180180

181181
resolved_version = version or resp.headers.get("x-schema-version", "latest")
@@ -215,10 +215,10 @@ async def _download_collection(
215215
else:
216216
resp = await client.get(_collection_url(base_url, namespace, name))
217217
if resp.status_code == 404:
218-
detail = "not found"
219-
with suppress(Exception):
220-
detail = resp.json().get("detail", detail)
221-
_fail(_ErrorClass.NOT_FOUND, f"Error: {detail}")
218+
_fail(
219+
_ErrorClass.NOT_FOUND,
220+
f"No collection named '{namespace}/{name}' found on {_host_from(base_url)}.",
221+
)
222222
resp.raise_for_status()
223223

224224
data = resp.json()

tests/unit/ctl/test_marketplace_app.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
from infrahub_sdk.ctl import config as ctl_config
99
from infrahub_sdk.ctl.marketplace import app
10+
from infrahub_sdk.ctl.marketplace import get as marketplace_get
1011

1112
runner = CliRunner()
1213
split_runner = CliRunner(mix_stderr=False)
@@ -604,3 +605,25 @@ def test_get_collection_stdout_separator(httpx_mock: HTTPXMock, tmp_path: Path)
604605

605606
assert result.exit_code == 0
606607
assert result.stdout == bare_yaml + "---\n" + bare_yaml
608+
609+
610+
async def test_collection_false_downloads_schema(httpx_mock: HTTPXMock, tmp_path: Path) -> None:
611+
"""collection=False is unreachable from CLI but defensively treated as a schema download."""
612+
httpx_mock.add_response(
613+
method="GET",
614+
url="https://marketplace.infrahub.app/api/v1/schemas/acme/network-base/download",
615+
text=SCHEMA_YAML,
616+
headers={"x-schema-version": "1.0.0"},
617+
)
618+
619+
await marketplace_get(
620+
identifier="acme/network-base",
621+
version=None,
622+
collection=False,
623+
stdout=False,
624+
output_dir=tmp_path,
625+
marketplace_url="https://marketplace.infrahub.app",
626+
_="",
627+
)
628+
629+
assert (tmp_path / "network-base.yml").read_text() == SCHEMA_YAML

0 commit comments

Comments
 (0)