-
Notifications
You must be signed in to change notification settings - Fork 154
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
Integration test for Google GCS hosted table #105
Conversation
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.
- Could you also add a quick example in https://github.com/delta-io/delta-sharing/blob/main/server/src/universal/conf/delta-sharing-server.yaml.template#L14 ?
- For the test, I think we also need to write code to generate the file that
GOOGLE_APPLICATION_CREDENTIALS
points to.
Added.
Why can not we just setup the file path? |
GitHub Actions runs in a container, but we don't want to build a public docker image that includes the google key file. To be clear, I meant the following steps:
|
@zsxwing I added code to write temp file with key body loaded from github secret in integration test and it is verified to work (see https://github.com/delta-io/delta-sharing/tree/integration-test). However there is another issue of |
We can ignore the mypy check error by adding a comment like this:
|
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.
Left some minor style comments. Otherwise LGTM
spark/src/test/scala/io/delta/sharing/spark/DeltaSharingSuite.scala
Outdated
Show resolved
Hide resolved
server/src/test/scala/io/delta/sharing/server/DeltaSharingServiceSuite.scala
Outdated
Show resolved
Hide resolved
server/src/test/scala/io/delta/sharing/server/DeltaSharingServiceSuite.scala
Outdated
Show resolved
Hide resolved
server/src/test/scala/io/delta/sharing/server/DeltaSharingServiceSuite.scala
Outdated
Show resolved
Hide resolved
@zsxwing All comments addressed and integration tests are running correctly for server, spark and python client. Please take a final look. |
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
The python integration test does not work yet. Still looking.