Uh oh!
There was an error while loading. Please reload this page.
feat: Add REST loadCredentials support - #3499
Conversation
9cb70c8 to
69a6e5bCompare69a6e5b to
64d39b6Compare| 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: planIdin: queryrequired: falseschema:
type: stringdescription: 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
gabeiglio
commented
Jun 15, 2026
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
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
kevinjqliu
commented
Jun 20, 2026
yep! this PR + #3500 wires up the component to use the |
kevinjqliu
commented
Jun 20, 2026
thanks for the review everyone! |
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