Adding support for Google AuthManager#2072
Conversation
Fokko
left a comment
There was a problem hiding this comment.
Thanks for adding this @rambleraptor 🙌
I'm not too familiar with the Google Auth system. WDYT @talatuyarer?
Let me try the PR on my local machine @Fokko |
|
@talatuyarer Any luck? :) |
|
@rambleraptor Could you update your PR with latest master changes |
39f5f22 to
1567dca
Compare
|
@talatuyarer changes updated to main branch! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Google authentication support to the Iceberg Python client by implementing a GoogleAuthManager class, matching functionality from a corresponding Java PR. The implementation provides convenient authentication methods for GCP users including Application Default Credentials (ADC) support and service account key file authentication.
- Implements
GoogleAuthManagerwith support for default credentials and service account key files - Adds comprehensive unit tests covering different authentication scenarios
- Configures optional
google-authdependency and mypy settings
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pyiceberg/catalog/rest/auth.py | Implements GoogleAuthManager class with Google authentication support |
| tests/catalog/test_rest_auth.py | Adds comprehensive unit tests for GoogleAuthManager functionality |
| pyproject.toml | Adds optional google-auth dependency and mypy configuration |
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! (and to copilot)
|
@talatuyarer could you take another look before I merge? |
b7222b9 to
c915752
Compare
|
@rambleraptor looks like there a merge conflict with main |
Fokko
left a comment
There was a problem hiding this comment.
This looks great @rambleraptor
talatuyarer
left a comment
There was a problem hiding this comment.
@rambleraptor I tested this PR. It is working as expected. Please update documentation. Beside of that LGTM!
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
c915752 to
c958c5f
Compare
|
@Fokko @kevinjqliu @talatuyarer docs updated + merge conflict removed! |
sungwy
left a comment
There was a problem hiding this comment.
awesome work @rambleraptor ! I'm running the CI, I will merge it once the CI passes
| self._auth_request = google.auth.transport.requests.Request() | ||
|
|
||
| def auth_header(self) -> str: | ||
| self.credentials.refresh(self._auth_request) |
There was a problem hiding this comment.
nit: is this .refresh called on every request?
Rationale for this change
Matches this Java PR
Google's authentication for BigQuery involves using different authentication methods than the current AuthManagers support. This allows users to authenticate against the BigLake REST Catalog (formerly known as the BigQuery REST Catalog)
The GoogleAuthManager introduces authentication methods that are more convenient for GCP users, differing from the generic OAuth2 flow:
Are these changes tested?
Unit tests included.
Are there any user-facing changes?
Adds support for Google authentication