-
Notifications
You must be signed in to change notification settings - Fork 120
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 for configuring Admin API URL #1617
Conversation
Note: I will update docs/jdbc.md once the PR #1605 is merged. |
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.
Do we want to keep service path? I believe we don't use that in any meaningful way in this project or any other connectors.
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've requested some minor changes to the documentation. The implementation looks great! Nice work.
private static final Pattern GENERATE_EPHEMERAL_CERT_PATTERN = | ||
Pattern.compile( | ||
".*/sql/v1beta4/projects/(?<project>.*)/instances/(?<instance>.*):generateEphemeralCert"); | ||
"(?<baseUrl>.*)sql/v1beta4/projects/(?<project>.*)/instances/(?<instance>.*):generateEphemeralCert"); |
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.
Nice. This is as good as an integration test, since it actually exercises the http client.
return adminRootUrl; | ||
} | ||
|
||
public String getlAdminServicePath() { |
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 think this is a typo. Maybe it should be getAdminServicePath
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.
Thank you for catching that. Updated.
docs/r2dbc.md
Outdated
### Example | ||
|
||
```java | ||
ConnectionFactoryOptions options=ConnectionFactoryOptions.builder() |
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.
More realistic example strings for the docs:
"https://googleapis.example.com/"
"sqladmin/v1/"
Also, we may want to direct people to see the javadoc for setRootUrl
and setServicePath
: https://cloud.google.com/java/docs/reference/google-api-client/latest/com.google.api.client.googleapis.services.AbstractGoogleClient.Builder#com_google_api_client_googleapis_services_AbstractGoogleClient_Builder_setRootUrl_java_lang_String_
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.
Updated.
b57b171
to
33c40a9
Compare
I don't have a preference. I could remove it. @hessjcg what do you think? |
For historical reasons we've always used v1beta4, even though there's a v1. This is to ensure v1beta4 continues to work, given the significant customer usage. |
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. Nice work!
I think we provide |
Fixes #1587