POC Quota API SDK/CLI#1217
Conversation
|
@asonnenschein to do - add tests. |
asonnenschein
requested review from
bcvazquez,
Copilot,
ischneider and
tbarsballe
There was a problem hiding this comment.
Pull request overview
Adds first-pass SDK + CLI support for Planet’s Quota Reservations API, including an async client, sync wrapper on Planet, and a new planet quota CLI command group, plus integration tests for both API and CLI behavior.
Changes:
- Introduces
QuotaClient(async) with pagination support for the Quota API response shape. - Adds
QuotaAPI(sync) and exposes it asPlanet().quota. - Adds
planet quotaCLI group (products/reservations/jobs) and integration tests covering expected request/response behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/test_quota_cli.py | New integration coverage for planet quota CLI subcommands and request parameter handling. |
| tests/integration/test_quota_api.py | New integration coverage for async + sync Quota API clients, pagination, and product filtering. |
| planet/sync/quota.py | New synchronous wrapper (QuotaAPI) around the async QuotaClient. |
| planet/sync/client.py | Exposes QuotaAPI as Planet().quota. |
| planet/clients/quota.py | New async QuotaClient + _QuotaPaged pagination adapter for {meta, results} responses. |
| planet/clients/init.py | Registers and exports QuotaClient for sess.client('quota') resolution. |
| planet/cli/types.py | Adds mypy override ignores for CommaSeparatedString/Float.convert signatures. |
| planet/cli/quota.py | New planet quota CLI group implementation using the async client. |
| planet/cli/cli.py | Registers the new quota CLI command group. |
| planet/init.py | Exports QuotaClient from the top-level planet package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from typing import Any, AsyncIterator, Dict, List, Optional, TypeVar | ||
|
|
||
| from planet.clients.base import _BaseClient | ||
| from planet.exceptions import APIError, ClientError | ||
| from planet.http import Session | ||
| from planet.models import Paged, Response | ||
| from ..constants import PLANET_BASE_URL | ||
|
|
||
| BASE_URL = f'{PLANET_BASE_URL}/account/v1' | ||
|
|
||
| LOGGER = logging.getLogger() | ||
|
|
||
| T = TypeVar("T") |
| def _aoi_refs_from_options(aoi_ref: Tuple[str, ...], | ||
| aoi_refs_json: Optional[List[str]]) -> List[str]: | ||
| refs: List[str] = list(aoi_ref) | ||
| if aoi_refs_json: | ||
| refs.extend(aoi_refs_json) | ||
| if not refs: | ||
| raise click.BadParameter( | ||
| 'At least one AOI ref must be supplied via --aoi-ref or ' | ||
| '--aoi-refs.') | ||
| return refs |
tbarsballe
left a comment
tbarsballe
left a comment
There was a problem hiding this comment.
Overall, this looks good. I've run some test commands locally, and everything works how I'd expect (though I didn't actually try reserving anything).
There's a little bit of an issue with verbose structure + vague or redundant comments throughout the MR, that is common with AI code but it's not too bad there. I've called out a couple places where it's noticeably inconsistent.
Following on from the discussion points during last weeks office hours: I ran quota reservations list on a customer account, and can confirm non-null organization_ids are returned in the results - it looks like the weirdness you were seeing when testing is a quirk of org 1.
| fields: Optional[str] = None, | ||
| sort: Optional[str] = None, | ||
| filters: Optional[Dict[str, Any]] = None, | ||
| page_size: Optional[int] = None, |
There was a problem hiding this comment.
Do we want to set a default here? Especially since this is only really relevant to the internal performance, as end-users just get an iterator. Subscriptions uses 500, for example.
| from planet.models import Paged, Response | ||
| from ..constants import PLANET_BASE_URL | ||
|
|
||
| BASE_URL = f'{PLANET_BASE_URL}/account/v1' |
There was a problem hiding this comment.
I don't think the way this was done is necessarily wrong or needs to be changed, but strictly speaking the base url for the Quota API (following what's documented in the API reference) is f'{PLANET_BASE_URL}/account/v1/quota-reservations'
It looks like this is using the parent URL so that it can also accommodate the /my/products endpoint, which is technically a different API. It is possible in the future we may have need of other features under the /my/ endpoint elsewhere. We can probably just refactor if it ever comes up, but I wanted to at least point it out.
| ) -> Iterator[dict]: | ||
| """Iterate over quota reservations. | ||
|
|
||
| See [QuotaClient.list_reservations][planet.clients.quota.QuotaClient.list_reservations] |
There was a problem hiding this comment.
Should probably verify these links work as expected after deploying the docs - the only other place I found links is in subscription_request and they're using a slightly different syntax
|
|
||
| # --------------------------------------------------------------------------- | ||
| # list_reservations | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
This does not match the structure of comments elsewhere in our test code, and seems to be a bit of an AI-code smell.
|
|
||
| @respx.mock | ||
| async def test_list_reservations_respects_limit(): | ||
| # Two pages of two items each — limit=3 cuts iteration short. |
There was a problem hiding this comment.
Nitpick: This should maybe be a pydoc (""") rather than an inline comment, especially since the line that actually does this is 122, not 120
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Products | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
This comment structure is not found elsewhere in our codebase, and doesn't add much of value. Remove?
Description
Adds support for the Planet Quota Reservations API to the SDK. Includes an async client, sync client, and CLI built on the
async client, following the same patterns used by the existing Planet APIs that are supported in the SDK.
Changes
QuotaClient(planet/clients/quota.py) covering all Quota Reservations API endpoints:list_reservations,get_reservation,create_reservation,bulk_create_reservations,estimate_reservation,list_jobs,get_job, pluslist_products(with optionalsupports_reservationfilter) for looking up theproduct_idrequired by reservation requests._QuotaPagedsubclass ofmodels.Pagedto handle the API's{meta: {next: ...}, results: [...]}pagination shape.QuotaAPI(planet/sync/quota.py) synchronous wrapper, exposed onPlanetaspl.quota.planet quotaCLI group (planet/cli/quota.py) withproducts,reservations, andjobssubcommands. List commands support--limit,--page-size,--sort,--fields, and repeated--filter KEY=VALUE. Mutation commands accept repeated--aoi-refor--aoi-refs <json>(string, file, or stdin) for large AOI lists.QuotaClientinplanet.clients._client_directorysosess.client('quota')resolves it, and exportQuotaClientfrom the top-levelplanetpackage.