-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/add support kerberize hivemetastore #1634
Feat/add support kerberize hivemetastore #1634
Conversation
…-support-kerberize-hivemetastore
e6610b1
to
3ab287f
Compare
3ab287f
to
38d9c2c
Compare
It is not maintained anymore: https://github.com/apple/ccs-pykerberos |
i think to get around this, we need to install some packages on the github runner os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that we dont really have a way to verify that this implementation is correct
pyproject.toml
Outdated
@@ -80,6 +80,8 @@ sqlalchemy = { version = "^2.0.18", optional = true } | |||
getdaft = { version = ">=0.2.12", optional = true } | |||
cachetools = "^5.5.0" | |||
pyiceberg-core = { version = "^0.4.0", optional = true } | |||
thrift-sasl = { version = ">=0.4.3", optional = true } | |||
kerberos = { version = "1.3.1", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is causing the CI issue. Im not sure if we really require this dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, let me check! I was surprised by how little there was as an alternative to this dependency, it is pretty likely that it is not needed at all.
…-support-kerberize-hivemetastore
return _HiveClient( | ||
uri, | ||
properties.get("ugi"), | ||
property_as_bool(properties, HIVE_KERBEROS_AUTH, HIVE_KERBEROS_AUTH_DEFAULT), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is causing a few tests to fail, we need to add the extra arg to the tests
like so https://github.com/kevinjqliu/iceberg-python/pull/10/files#diff-8a0f847796be6745b3be158f5c39d0e83cbb868c162763a13895175b484cc529
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinjqliu all green :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Let's merge this and iterate if necessary
Took #766 and addressed the comments, to make sure that it gets into 0.9.0