-
Notifications
You must be signed in to change notification settings - Fork 43
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: multiple dbs support #928
Conversation
Warning: This pull request is touching the following templated files:
|
datastore-v1-proto-client/src/test/java/com/google/datastore/v1/client/QuerySplitterTest.java
Show resolved
Hide resolved
google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java
Outdated
Show resolved
Hide resolved
...proto-client/src/test/java/com/google/datastore/v1/client/it/ITDatastoreProtoClientTest.java
Show resolved
Hide resolved
google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java
Outdated
Show resolved
Hide resolved
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.
@kolea2 awesome job with troubleshooting and addressing the native image issues! I've added some comments about naming and the location of the config file but otherwise looks great.
@@ -30,4 +30,9 @@ env_vars: { | |||
env_vars: { | |||
key: "SECRET_MANAGER_KEYS" | |||
value: "java-it-service-account" | |||
} | |||
|
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.
Let's also merge this PR with main
so that it is tested with GraalVM 22.3.0. gcr.io/cloud-devrel-kokoro-resources/graalvm17
was recently upgraded to gcr.io/cloud-devrel-kokoro-resources/graalvm17:22.3.0
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.
will do - I will have a followup PR updating this branch, so stay tuned!
.../src/test/resources/META-INF/native-image/com/google/datastore/v1/client/reflect-config.json
Outdated
Show resolved
Hide resolved
.../src/test/resources/META-INF/native-image/com/google/datastore/v1/client/reflect-config.json
Outdated
Show resolved
Hide resolved
@@ -29,5 +29,55 @@ | |||
"allPublicMethods":true, | |||
"allDeclaredConstructors" : true, | |||
"allPublicConstructors" : 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.
Ah looks like we have an extra [ ]
that's causing the graalvm job failure.
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 catch, thanks!
This PR consists of cherry picks from the https://github.com/googleapis/java-datastore/tree/multi-db branch. They include: #928 #940 #942 This also enables parameterized testing for ITDatastoreTest.
No description provided.