feat: Add REST loadCredentials support#3499
Conversation
kevinjqliu
force-pushed
the
codex/load-credentials-api
branch
from
9cb70c8 to
69a6e5b
Compare
kevinjqliu
force-pushed
the
codex/load-credentials-api
branch
from
69a6e5b to
64d39b6
Compare
kevinjqliu
changed the title
|
|
||
| return ScanTasks.model_validate_json(response.text) | ||
|
|
||
| @retry(**_RETRY_ARGS) |
There was a problem hiding this comment.
nit: These methods are added between _fetch_scan_tasks and plan_scan. These two methods are closely related. How about relocating these new methods to different locations?
There was a problem hiding this comment.
Yeah I mean they could be moved closer to the tables methods so it's easier to find. But just a nit lmk wyt @kevinjqliu
There was a problem hiding this comment.
good eye! made the change, ty both!
| response = self._session.get( | ||
| self.url(Endpoints.load_credentials, prefixed=True, **self._split_identifier_for_path(identifier)), |
There was a problem hiding this comment.
Assuming we'll support optional parameters as a follow-up if necessary:
parameters:
- name: planId
in: query
required: false
schema:
type: string
description: The plan ID that has been used for server-side scan planning
- $ref: '#/components/parameters/referenced-by'There was a problem hiding this comment.
yep i purposely left it out to keep the PR smaller
|
This endpoint is somewhat ambiguous overall. It appears in the REST spec but is not implemented in the REST catalog (at least in Java). In practice, credential loading is handled directly by VendedCredentialsProvider.java, where IRC passes the URI and endpoint for requesting new credentials through the config. This PR will also determine how the work I’m doing will invoke the credentials endpoint to obtain new credentials. |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
I'd love to see planId as a fast-follow, but this looks good as-is.
|
|
||
| return ScanTasks.model_validate_json(response.text) | ||
|
|
||
| @retry(**_RETRY_ARGS) |
There was a problem hiding this comment.
Yeah I mean they could be moved closer to the tables methods so it's easier to find. But just a nit lmk wyt @kevinjqliu
yep! this PR + #3500 wires up the component to use the |
|
thanks for the review everyone! |
5 participants
Summary
loadCredentialsendpoint supportLoadCredentialsResponseRestCatalog.load_credentials(...)with longest-prefix resolution for a target locationContributes to #3495.
Notes
The pinned
apache/iceberg-rest-fixture:1.10.1used by integration tests does not serve the/credentialsroute, so this is covered with REST catalog unit tests rather than fixture-backed integration tests.Testing
PYTHONPATH=. pytest tests/catalog/test_rest.py -k 'load_credentials or storage_credentials'ruff check pyiceberg/catalog/rest/__init__.py tests/catalog/test_rest.pyruff format --check pyiceberg/catalog/rest/__init__.py tests/catalog/test_rest.py