feat: Rework the Python SDK - #7
Conversation
bd4ee43 to
158c720
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Non-2xx handling, qualified bulk routing, and union generation contain correctness defects, while the required generator dependency remains unmerged.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reworks the repository into an async Python SDK with generated OpenAPI plumbing and an idiomatic multi-metro resource layer.
Changes:
- Replaces the legacy generated SDK with Pydantic-based platform and control-plane clients.
- Adds pagination, fan-out, resource handles, patching, and structured errors.
- Adds comprehensive tests, examples, documentation, generation, CI, and publishing automation.
File summaries
| File | Description |
|---|---|
.editorconfig, .gitattributes |
Adds repository formatting attributes. |
.github/workflows/{actionlint,check-pr,ci,publish-pypi,release-stable,sync}.yaml |
Updates validation, generation, release, and publishing automation. |
.platform-config.yaml |
Removes obsolete platform configuration. |
Makefile |
Replaces generation and development commands. |
README.md |
Documents the rewritten SDK and APIs. |
examples/{plumbing,quickstart,update}.py |
Demonstrates raw and idiomatic usage. |
pyproject.toml, uv.lock |
Defines package metadata and locked dependencies. |
src/unikraft_cloud/{__init__,client}.py, py.typed |
Adds the public package and client entry point. |
src/unikraft_cloud/api/**/*.py |
Adds generated platform/control-plane clients and models. |
src/unikraft_cloud/core/*.py |
Implements transport, errors, pagination, fan-out, handles, patching, routing, and sessions. |
src/unikraft_cloud/resources/*.py |
Adds idiomatic resource clients. |
templates/models.py.tmpl, templates/resources.tmpl |
Adds Pydantic model and operation generation. |
templates/**/*.jinja |
Removes the legacy generator templates. |
tests/{__init__,conftest,test_client,test_core,test_resources,test_transport}.py |
Adds offline behavioral coverage. |
unikraft_cloud_platform/**/*.py, unikraft_cloud_platform/py.typed |
Removes the legacy generated SDK. |
Review details
Suppressed comments (2)
src/unikraft_cloud/core/http.py:431
- Malformed bodies on 3xx responses are classified as parse failures before
_raise_for_statusruns. Redirects are non-2xx HTTP failures under this client's contract, so this error-path check also needs to use the 2xx success range.
src/unikraft_cloud/core/http.py:446 - This treats every 3xx response as success. Since redirects are not followed by default, a redirect whose body happens to validate can be returned as a successful API result, and no-content operations silently accept it, despite
ApiClientpromising errors for all non-2xx statuses. Only 200–299 should return here.
- Files reviewed: 147/349 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
158c720 to
ddf47bf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Metro-scope handling, partial lookup failures, mutable lazy edits, path encoding, and generated GET-body handling can produce incorrect requests or target the wrong resource.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/unikraft_cloud/core/resource.py:98
- An explicitly supplied
metros=[]is discarded here because it is falsy, so a single-target operation can run in the default metro instead of rejecting a scope that selects no metros. Preserve empty sequences and fall back only when the option is absent.
- Files reviewed: 147/349 changed files
- Comments generated: 5
- Review effort level: Balanced
ddf47bf to
8460cfb
Compare
Signed-off-by: Alexander Jung <alex@unikraft.com>
Signed-off-by: Alexander Jung <alex@unikraft.com>
The SDK is async-only, so httpx supplies the transport and pydantic the models generated from the OpenAPI specification. Python 3.10 is the floor: the oldest release still supported upstream, and the first with `X | Y` annotations usable at runtime. Ruff's line length matches the JavaScript SDK's Biome configuration so the two SDKs read alike. The generated plumbing under `api/` mirrors the specification rather than the house style, so naming and line-length rules are relaxed there. Signed-off-by: Alexander Jung <alex@unikraft.com>
Signed-off-by: Alexander Jung <alex@unikraft.com>
Both plumbing clients come from the generator the JavaScript SDK uses, so the spec URLs, channel variable and `-v package=api` flag are kept identical; only the templates and output path differ. Generated sources are formatted after generation rather than emitted pre-formatted, which keeps the templates readable. Signed-off-by: Alexander Jung <alex@unikraft.com>
Publishing uses PyPI Trusted Publishing, so no API token is stored. The sync job only stages the generated `api/platform` and `api/controlplane` trees: everything else in the tree is hand-written and must not be touched by a regeneration. Signed-off-by: Alexander Jung <alex@unikraft.com>
One base error means `except UnikraftCloudError` catches every failure however the request went wrong, and `kind` says which layer failed. The status subclasses sit on top of it, so matching on a class and matching on `err.status` both work. Errors live apart from the transport because the fan-out and resource layers raise them too, and nothing there should have to import httpx. The envelope readers are deliberately defensive: on the error path the body may not match the specification, since that is often what failed. Signed-off-by: Alexander Jung <alex@unikraft.com>
Every generated client sends through this, so authentication, query encoding, envelope parsing and error mapping are decided once. An injected httpx client is shared rather than owned, so one session can hand the same connection pool to all of its resource clients and close it exactly once. Passing a transport instead is the test seam. The default timeout bounds connecting but not reading: the platform API's `wait` operations block for as long as the caller asked, and a read timeout would cut them short. Booleans are spelled lower-case and lists become repeated parameters, which is how the API spells its filters; `str(True)` would send `True` and be rejected. Signed-off-by: Alexander Jung <alex@unikraft.com>
A metro is just a string so a newly launched one works without an SDK upgrade; the known codes are listed for discoverability only. A value that is already a URL is honoured verbatim, which is how a staging or self-hosted deployment is reached, and a trailing `/v1` is dropped because the generated operation paths already carry it. Results say which metro served them. The generated models mirror the wire format and have no such field, so `with_metro` re-badges one as the hand-written subclass that adds it. It builds without validating: the value was already validated when parsed, and revalidating every row of a fan-out would cost real time on large listings. Undeclared fields are carried across so a tagged result is never lossier than the raw one. Signed-off-by: Alexander Jung <alex@unikraft.com>
`uuid` and `name` are mutually exclusive because the API validates whichever field it is given: a name in the `uuid` filter comes back as `Invalid uuid`. A reference also carries the metro it means, since a name is only unique within one, and that never reaches the wire -- it says where to send the request, and the API rejects unknown fields. The transport raises on HTTP failures, but the API also reports logical failures inside a 200 envelope, including bulk operations that only partly succeeded. Those are read field by field rather than through `model_dump()`: the check runs on every response, and dumping a page of listings to inspect two fields is wasteful. Signed-off-by: Alexander Jung <alex@unikraft.com>
The platform API is metro-scoped, so an account-wide read means asking every metro and merging the answers. Each metro keeps exactly one step in flight, and the next one is queued before the current item is handed over, so a slow metro never holds up a fast one and the interleaving is arrival order. One step is taken per pass rather than draining everything that finished, so nothing is lost when the consumer stops iterating part-way through. A failure does not end the merge: every healthy metro is drained and the failures are reported together afterwards, because a partial answer is more useful than none. Breaking out early cancels the rest and reports nothing -- the caller already stopped caring. A lone metro is passed straight through so its errors arrive as themselves instead of wrapped in an aggregate. Signed-off-by: Alexander Jung <alex@unikraft.com>
Single-resource operations hand back a handle rather than a coroutine so they compose: `get(name="web").suspend()` reads as one thought. The handle is awaitable too, so `await get(name="web")` still yields the instance and nothing needs a terminal call. Locating and reading are memoised as tasks, so several awaits and several chained operations share one lookup. A chained operation reuses the metro its parent resolved to, which is what keeps `get(ref).suspend()` down to a single request; only handles that are themselves an operation make the next link wait for them. A dropped handle sent nothing, and unlike a dropped coroutine nothing in the language notices, so it says so on collection. Chaining consumes the parent, so a forgotten `await` on a chain warns once at its end rather than once per link. Signed-off-by: Alexander Jung <alex@unikraft.com>
A name is unique within a metro, not across them, so the same name routinely refers to a resource in every metro. Addressing all of them is a deliberate choice rather than guessing one or refusing. Each metro's outcome comes back as a list of nought or one so that a success and a failure are both plain values: gather keeps handle order, which callers correlate with `where()`, and `None` stays a legitimate result instead of standing in for failure. Signed-off-by: Alexander Jung <alex@unikraft.com>
The API models an update as `{prop, op, value}` triples with an untyped
value: precise on the wire, unchecked and clunky to write. Properties
become keyword arguments instead, and both forms compile to the same
triples sent in one request.
Clearing a property needs its own value. JavaScript uses `null`, but in
Python `None` is how a caller spells "no opinion", and keyword arguments
give no way to tell an omitted property from one set to nothing -- so
REMOVE says it outright.
`delete` rather than `del`, which is a keyword.
Signed-off-by: Alexander Jung <alex@unikraft.com>
The platform API pages with a `count` size and a `from` cursor, and a short page marks the end. An item with no usable cursor also ends the iteration, rather than asking for the same page forever. Signed-off-by: Alexander Jung <alex@unikraft.com>
Generated request models cannot go through `json.dumps`, and dumping them needs two decisions. Aliases are applied, so a field named after a Python keyword still reaches the wire under its real name. Fields the caller never set are dropped, because the API distinguishes an absent field from a null one and a model's defaults are the SDK's opinion rather than the caller's. The absent-argument sentinel becomes public: generated operations take a per-call timeout, where `None` means "no timeout" and omitting it means "use the client's", and no generated file should reach for a private name to say the latter. Signed-off-by: Alexander Jung <alex@unikraft.com>
The specification declares `metro` on most resources but only fills it in for requests that went through the global control plane, so tagging a result passed the field twice and failed outright. The metro that actually served the response is always known, so it takes precedence over whatever the payload claimed. Signed-off-by: Alexander Jung <alex@unikraft.com>
Python evaluates a module top to bottom where TypeScript's declarations are order-free, so the output is laid out to survive the parser's alphabetical order: self-contained enum aliases first, then the models with deferred annotations, then one rebuild pass. A reference that cannot be resolved then fails on import rather than at the first request. Enums stay `Literal` aliases rather than enum classes so their values are plain strings and comparing against one needs no import. A property named after a Python keyword -- `from`, the pagination cursor -- is renamed and given an alias, so it stays addressable in Python while reaching the wire under its real name. Unknown fields are kept: the server may send more than the specification describes, and dropping it would make the client lossier than the wire. Signed-off-by: Alexander Jung <alex@unikraft.com>
Both specifications are generated in the proto repository before they reach the openapi one, so pointing at a local checkout of it is how a change is picked up before it has synced. Signed-off-by: Alexander Jung <alex@unikraft.com>
One class per tag, one method per operation, returning the envelope the specification describes. Parameters are keyword-only except for path parameters, which read better positionally. Bodies are dropped on GET and HEAD: no HTTP client will send one, and the specification exposes those filters as repeatable query parameters as well. An operation that answers with an event stream has no JSON body to return, so it yields the events instead. The docstrings are the specification's prose verbatim, non-ASCII characters and all, so the linter is told not to second-guess them. Signed-off-by: Alexander Jung <alex@unikraft.com>
Generated from the platform and control-plane specifications. Regenerate with `make generate`; nothing here is edited by hand. Signed-off-by: Alexander Jung <alex@unikraft.com>
Each surface gets a container so one set of credentials reaches all of its resources, and both sit behind `Api` as `ukc.api.platform` and `ukc.api.controlplane`. The two specifications name some types alike, so keeping the surfaces in separate packages is what stops them colliding. These are hand-written: regeneration only rewrites the `_gen` modules, so a newly tagged resource is added here deliberately. Signed-off-by: Alexander Jung <alex@unikraft.com>
A Python module cannot have a dot in its name, so the JavaScript SDK's `.gen.ts` becomes `_gen.py` and the markers have to match. The sync job narrows to those files too: the containers beside them are hand-written and a regeneration must not stage them. Signed-off-by: Alexander Jung <alex@unikraft.com>
One session holds the credentials and the metro map for a client, so
scoped clients like `ukc.metro("fra")` reuse both instead of duplicating
them.
Discovery runs once and is shared by concurrent callers, but a failed
attempt is not cached: a control-plane blip would otherwise poison the
client for its whole lifetime. Naming metros skips discovery entirely,
which is the other reason to name them.
The reported endpoint is trusted over one built from the code, so a new
or relocated metro works without an SDK release. Codes are lower-cased
because the control plane reports them upper-case while `metro("fra")`
is how they are written, and a result should be tagged the same whether
it was reached by discovery or by name.
An explicitly named endpoint collapses every scope to itself: there is
nothing to discover, and fanning out would invent hostnames the caller
never mentioned.
Signed-off-by: Alexander Jung <alex@unikraft.com>
A metro-scoped API poses two problems the resource clients all share: finding which metro holds a named resource, and turning a bulk operation into one call per metro. Locating sends nothing when the answer is already known -- a reference that names its metro, or a scope of exactly one -- which is what keeps `get(name="web").suspend()` to a single request. Otherwise every metro is asked at once and the resource that was found travels back with the target, so it is not read twice. A name matching in several metros raises rather than picking one, since the caller may be about to mutate it, and the matches ride along on the error so recovering costs no further requests. Finding nothing while a metro was unreachable is reported as that, not as a bare 404 the caller cannot act on. Each client holds its plumbing client rather than extending it, so short verbs and raw operations stay visibly separate. Signed-off-by: Alexander Jung <alex@unikraft.com>
`size()` asks how many metros hold a match without acting on any of them, so the handles it looked at were never going to send anything and reporting them as dropped un-awaited was a false alarm. Reading a resource takes plain call options too: the metro is already resolved by then, so a scope would be meaningless. Signed-off-by: Alexander Jung <alex@unikraft.com>
Envelope-free results, automatic pagination and metro fan-out, with single-instance operations returning a chainable handle. Properties are keyword arguments rather than a dict, so an update reads as `update(memory_mb=512)`; the raw triples stay reachable through `patch()` for what that cannot express. Each response type gets its own tagged subclass. The generated models know nothing of metros, and a subclass is what makes `inst.metro` a plain `str` that tools can see. `Instances.list` shadows the builtin inside the class body, so the annotations there name the builtin explicitly rather than renaming the method away from the verb every other SDK uses. Signed-off-by: Alexander Jung <alex@unikraft.com>
Account-wide by default: reads cover every metro and merge into one stream. Narrowing with `metro()`, `metros=` or the constructor is both the way to target one and the way to skip discovery. One httpx client is built for the whole session and shared by every resource client, so connections are pooled once and there is a single thing to close. Supplying your own hands its lifetime back to you. Credentials and the metro fall back to UKC_TOKEN and UKC_METRO, so a script needs no arguments at all. Signed-off-by: Alexander Jung <alex@unikraft.com>
Collecting per-call options, turning a reference into a body item or a query filter, and tagging a result are identical for every resource, and four more clients were about to copy them. A request that names another resource takes a bare string as its name, since that is what people type, and a reference when the caller knows which identifier they hold. Signed-off-by: Alexander Jung <alex@unikraft.com>
Attach and detach name the instance rather than a raw request item, and a create reads the volume back rather than carrying the create's answer forward: the API reports less on a create than on a read. Signed-off-by: Alexander Jung <alex@unikraft.com>
Signed-off-by: Alexander Jung <alex@unikraft.com>
A certificate is not updated property by property like the others: the API replaces the chain and key together, so `update` takes both and there is no staged editor. Signed-off-by: Alexander Jung <alex@unikraft.com>
Quotas are per-metro, so an account-wide view asks each metro and collects the answers. A metro that fails does not lose the others: the quotas that did arrive ride along on the error. Signed-off-by: Alexander Jung <alex@unikraft.com>
All five resources hang off any scope, so `ukc.volumes`, `ukc.services`, `ukc.certificates` and `ukc.users` work the same whether the client is account-wide or pinned to one metro. Signed-off-by: Alexander Jung <alex@unikraft.com>
`httpx.MockTransport` is the seam: the whole suite runs offline, and what reached the wire is asserted from the recorded requests. Fixtures carry every field the specification marks required, because validation is strict and a thin fixture fails for the wrong reason. The pydantic mypy plugin goes in alongside them: without it, building a model by its Python field name rather than its wire name looks like an error to the type checker. Signed-off-by: Alexander Jung <alex@unikraft.com>
These encode the decisions the resource clients rest on: that a located resource is not read twice, that a chain costs one lookup, that a slow metro never holds up a fast one, that the healthy metros are drained before a fan-out failure is reported, and that breaking out early releases the rest. The never-awaited warning is tested from both sides -- a dropped chain warns once at its end, an awaited one says nothing -- because a warning that cried wolf would be worse than none. Signed-off-by: Alexander Jung <alex@unikraft.com>
These are the behaviours the SDK exists for: that naming a metro skips discovery, that discovery happens once, that a name found in one metro is acted on only there, that a name in several is reported rather than guessed at, and that `each()` addresses them all. The per-resource tests pin down what actually reaches the wire, which is where the interesting differences are: a volume attach names an instance, a certificate is replaced whole rather than by property, and `from` travels under its real name. Signed-off-by: Alexander Jung <alex@unikraft.com>
Three, matching the JavaScript SDK's: the idiomatic quickstart, updating properties both ways, and the raw API on its own. They are type-checked along with the rest of the tree, so a signature that changes under them fails the build rather than the reader. Signed-off-by: Alexander Jung <alex@unikraft.com>
Written around the two decisions a reader trips over first: that a name is only unique within a metro, so addressing one across metros can be ambiguous on purpose, and that single-resource operations hand back a chainable handle rather than a coroutine. Signed-off-by: Alexander Jung <alex@unikraft.com>
8460cfb to
673266e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved transport, client-lifecycle, and cross-metro reference bugs can cause leaked resources, inconsistent exceptions, or requests against unintended resources.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/unikraft_cloud/resources/volumes.py:242
- A qualified
from_reference also loses itsmetroinname_or_uuid. DetachingRef(name="web", metro="dal")from a volume resolved in another metro can therefore detach the wrong local instance. Validate the qualifier againsttarget.metrobefore building this request.
- Files reviewed: 146/349 changed files
- Comments generated: 5
- Review effort level: Balanced
| async def run(target: MetroTarget) -> AttachedVolume: | ||
| body = [ | ||
| models.AttachVolumesRequestItem.model_validate( | ||
| {**ref_dict(target.ref), "attach_to": name_or_uuid(to), "at": at} | ||
| | ({} if readonly is None else {"readonly": readonly}) |
| def __init__(self, config: ApiClientConfig) -> None: | ||
| self.auth = AuthApi(config) | ||
| self.images = ImagesApi(config) | ||
| self.metros = MetrosApi(config) | ||
| self.node_activation = NodeActivationServiceApi(config) |
| def __init__(self, config: ApiClientConfig) -> None: | ||
| self.autoscale = AutoscaleApi(config) | ||
| self.certificates = CertificatesApi(config) | ||
| self.images = ImagesApi(config) | ||
| self.instances = InstancesApi(config) |
| merged: dict[str, str] = {"accept": accept, **self._default_headers} | ||
| if headers: | ||
| merged.update(headers) | ||
| # The token is applied last: a caller's own `authorization` header would | ||
| # otherwise silently deauthenticate the request. | ||
| if self._config.token: | ||
| merged["authorization"] = f"Bearer {self._config.token}" |
| try: | ||
| if not response.is_success: | ||
| await response.aread() | ||
| self._raise_for_status(response, self._parse_json(response, request.url)) | ||
|
|
||
| adapter = TypeAdapter(model) | ||
| buffer = "" | ||
| async for chunk in response.aiter_text(): | ||
| buffer += chunk | ||
| while match := _SSE_EVENT_BOUNDARY.search(buffer): | ||
| block, buffer = buffer[: match.start()], buffer[match.end() :] | ||
| event = self._parse_event(adapter, block, request.url, response.status_code) | ||
| if event is not None: | ||
| yield event | ||
| # A last event may arrive without its trailing blank line. | ||
| event = self._parse_event(adapter, buffer, request.url, response.status_code) | ||
| if event is not None: | ||
| yield event |
The Python SDK for the Platform and control-plane APIs, modelled on the JavaScript one. The repository was reset, so this is all of it new: templates, generated plumbing, hand-written runtime, resource clients, tests and docs.
Same two layers as the JavaScript SDK. Generated plumbing mirrors the specification and stays reachable on
ukc.api; the hand-written layer on top gives envelope-free results, pagination, chainable handles and multi-metro fan-out. Generation uses the sameopenapi-gen;_gen.pyfiles are never edited by hand.Async-only, since the JavaScript core ports across almost directly that way. Models are
pydanticgenerated against the spec as written, so required fields are actually required and unknown ones survive. Resources take keyword arguments (get(name="web")) and because a name is only unique within a metro, a name matching in several is reported rather than guessed at, witheach()for acting on all of them. Single-resource operations return a chainable handle that is alsoawaitable, and warns if it is dropped un-awaited, since nothing else would tell you no request was sent.Tests run offline through
httpx.MockTransport, green on 3.10–3.13, withmypystrict over source, tests and examples.Before merging
The generator's Python helpers are not upstream yet (see unikraft-cloud/x#TODO) — same situation as the TypeScript ones the JavaScript SDK already depends on. Until they land, make generate needs a local
openapi-genbuild; theMakefilesays how. Two additions were needed:anyOf/oneOfas Python unions, and not indenting blank lines in docstrings.GitHub-Depends-On: unikraft-cloud/x#385