-
Notifications
You must be signed in to change notification settings - Fork 62
feat: Add full support for Universe Domain #1604
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
Changes from all commits
823d03b
3d9822a
bdca46b
d93adfa
52f31af
a9a1f01
0c4e2da
c441f15
32fb943
2caf336
03b2bd3
f618252
f00d375
94926c8
f5bf88d
3cc6785
414cc28
7c3c431
30b2c48
2d4b4a3
36255ce
482e922
b0868ce
e4ccb2e
05a95c2
b1bea62
b52de15
a556b40
53867bd
334904c
800b55b
e44c1da
2cd27dd
3ec698d
0096646
ee50b02
07b988e
8922bc9
69484f1
ab8c15d
de5c7b6
b7115bb
31f7274
cbff5f5
60461b3
9a55cce
3cb4820
024027d
35ff346
00396b6
f12b686
a518dff
8fb5b32
d16758c
761ccd2
56b2331
001aeaf
e5993ab
5418648
952fc80
bea59c3
8c8b988
5a989a4
96d69ec
3a058b6
524ae61
76e8b65
7444993
23c52b4
b88c9b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,28 +104,81 @@ export interface BigtableOptions extends gax.GoogleAuthOptions { | |
} | ||
|
||
/** | ||
* Retrieves the domain to be used for the service path. | ||
* Retrieves the universe domain, if configured. | ||
* | ||
* This function retrieves the domain from gax.ClientOptions passed in or via an environment variable. | ||
* It defaults to 'googleapis.com' if none has been set. | ||
* @param {string} [prefix] The prefix for the domain. | ||
* @param {gax.ClientOptions} [opts] The gax client options | ||
* @returns {string} The universe domain. | ||
* This function checks for a universe domain in the following order: | ||
* 1. The `universeDomain` property within the provided options. | ||
* 2. The `universeDomain` or `universe_domain` property within the `opts` object. | ||
* 3. The `GOOGLE_CLOUD_UNIVERSE_DOMAIN` environment variable. | ||
* | ||
* If a universe domain is found in any of these locations, it is returned. | ||
* Otherwise, the function returns `undefined`. | ||
* | ||
* @param {BigtableOptions} options - The Bigtable client options. | ||
* @param {gax.ClientOptions} [gaxOpts] - Optional gax client options. | ||
* @returns {string | undefined} The universe domain, or `undefined` if not found. | ||
*/ | ||
function getDomain(prefix: string, opts?: gax.ClientOptions) { | ||
function getUniverseDomainOnly( | ||
options: BigtableOptions, | ||
gaxOpts?: gax.ClientOptions, | ||
): string | undefined { | ||
// From https://github.com/googleapis/nodejs-bigtable/blob/589540475b0b2a055018a1cb6e475800fdd46a37/src/v2/bigtable_client.ts#L120-L128. | ||
// This code for universe domain was taken from the Gapic Layer. | ||
// It is reused here to build the service path. | ||
const universeDomainEnvVar = | ||
typeof process === 'object' && typeof process.env === 'object' | ||
? process.env['GOOGLE_CLOUD_UNIVERSE_DOMAIN'] | ||
: undefined; | ||
return `${prefix}.${ | ||
opts?.universeDomain ?? | ||
opts?.universe_domain ?? | ||
universeDomainEnvVar ?? | ||
'googleapis.com' | ||
}`; | ||
return ( | ||
gaxOpts?.universeDomain ?? | ||
gaxOpts?.universe_domain ?? | ||
options?.universeDomain ?? | ||
universeDomainEnvVar | ||
); | ||
} | ||
|
||
/** | ||
* Retrieves the universe domain options from the provided options. | ||
* | ||
* This function examines the provided BigtableOptions and an optional | ||
* gax.ClientOptions object to determine the universe domain to be used. | ||
* It prioritizes the `universeDomain` property in the options, then checks | ||
* for `universeDomain` or `universe_domain` in the gax options, and finally | ||
* falls back to the `GOOGLE_CLOUD_UNIVERSE_DOMAIN` environment variable. | ||
* If a universe domain is found, it returns an object containing the | ||
* `universeDomain` property; otherwise, it returns `null`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it may simplify things to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason getUniverseDomainOptions returns null is that the return value is used in an So the behaviour we want that we are getting with this code is don't modify the gapic client options when null gets returned. |
||
* | ||
* @param {BigtableOptions} options - The Bigtable client options. | ||
* @param {gax.ClientOptions} [gaxOpts] - Optional gax client options. | ||
* @returns {{universeDomain: string} | null} An object containing the `universeDomain` property if found, | ||
* otherwise `null`. | ||
*/ | ||
function getUniverseDomainOptions( | ||
options: BigtableOptions, | ||
gaxOpts?: gax.ClientOptions, | ||
): {universeDomain: string} | null { | ||
const universeDomainOnly = getUniverseDomainOnly(options, gaxOpts); | ||
return universeDomainOnly ? {universeDomain: universeDomainOnly} : null; | ||
} | ||
|
||
/** | ||
* Retrieves the domain to be used for the service path. | ||
* | ||
* This function retrieves the domain from gax.ClientOptions passed in or via an environment variable. | ||
* It defaults to 'googleapis.com' if none has been set. | ||
* @param {string} [prefix] The prefix for the domain. | ||
* @param {BigtableOptions} [options] The options passed into the Bigtable client. | ||
* @param {gax.ClientOptions} [gaxOpts] The gax client options. | ||
* @returns {string} The universe domain. | ||
*/ | ||
function getDomain( | ||
prefix: string, | ||
options: BigtableOptions, | ||
gaxOpts?: gax.ClientOptions, | ||
): string { | ||
const universeDomainOnly = getUniverseDomainOnly(options, gaxOpts); | ||
const suffix = universeDomainOnly ? universeDomainOnly : 'googleapis.com'; | ||
return `${prefix}.${suffix}`; | ||
} | ||
|
||
/** | ||
|
@@ -467,10 +520,11 @@ export class Bigtable { | |
const dataOptions = Object.assign( | ||
{}, | ||
baseOptions, | ||
getUniverseDomainOptions(options, options.BigtableClient), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this, ie. without providing a universeDomain to the gapic client we see an error that says: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just be opinionated here and pass in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to pass the universe domain option through with the Env Var because that'll be done by the GAPICs. Even though you need it to calculate the servicePath, you don't need to set it as the UniverseDomain option as well. I would suggest taking the path of either 1. "configure the bare minimum and let the GAPICs do the rest" or 2. "always explicitly set it so the GAPICs do nothing". So this would be, either always ensure the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to choose option 1 because I want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For your suggestion 2, if the user provides the universe domain in an environment variable and it doesn't get provided to the Gapic client in universe_domain then I actually get an error. It says |
||
{ | ||
servicePath: | ||
customEndpointBaseUrl || | ||
getDomain('bigtable', options.BigtableClient), | ||
getDomain('bigtable', options, options.BigtableClient), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now supports the use case where the universe domain is provided in the handwritten client options as well as maintaining support for when the universe domain is provided just for the gapic client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this look like when the user provides universe domain in the GAPIC client but not in the handwritten client? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then it would be set to |
||
'grpc.callInvocationTransformer': grpcGcp.gcpCallInvocationTransformer, | ||
'grpc.channelFactoryOverride': grpcGcp.gcpChannelFactoryOverride, | ||
'grpc.gcpApiConfig': grpcGcp.createGcpApiConfig({ | ||
|
@@ -488,20 +542,26 @@ export class Bigtable { | |
const adminOptions = Object.assign( | ||
{}, | ||
baseOptions, | ||
getUniverseDomainOptions(options, options.BigtableTableAdminClient), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this, ie. without providing a universeDomain to the gapic client we see an error that says: |
||
{ | ||
servicePath: | ||
customEndpointBaseUrl || | ||
getDomain('bigtableadmin', options.BigtableClient), | ||
getDomain('bigtableadmin', options, options.BigtableTableAdminClient), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now supports the use case where the universe domain is provided in the handwritten client options as well as maintaining support for when the universe domain is provided just for the gapic client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this should be |
||
}, | ||
options, | ||
); | ||
const instanceAdminOptions = Object.assign( | ||
{}, | ||
baseOptions, | ||
getUniverseDomainOptions(options, options.BigtableInstanceAdminClient), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this, ie. without providing a universeDomain to the gapic client we see an error that says: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this should be BigtableInstanceAdminClient as this is a setting for the instance admin client. |
||
{ | ||
servicePath: | ||
customEndpointBaseUrl || | ||
getDomain('bigtableadmin', options.BigtableClient), | ||
getDomain( | ||
'bigtableadmin', | ||
options, | ||
options.BigtableInstanceAdminClient, | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now supports the use case where the universe domain is provided in the handwritten client options as well as maintaining support for when the universe domain is provided just for the gapic client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this should be BigtableInstanceAdminClient as this is a setting for the instance admin client. |
||
}, | ||
options, | ||
); | ||
|
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.
Why do we check the snake-case
universe_domain
for thegaxOpts
but not for theoptions
opts?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 something that Alex decided on a while back. I'm not entirely sure why we do it, but we do it in generated gapic clients too, so it's in all of the libraries. (At least the ones I've seen.)
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.
Should it be added to
options
then as well? e.g.Or should snake-case be marked deprecated so that newer clients (e.g. this one) can ignore it?
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.
So the BigtableOptions passed into the handwritten layer at https://github.com/googleapis/nodejs-bigtable/blob/main/src/index.ts#L82 extend GoogleAuthOptions and only support camel case according to the screenshot:

But the universe domain for the Gapic ClientOptions type supports both camel case and snake case:
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 have a suspicion that the reason we did this is that an earlier version of the spec wanted snake case, and then we decided to make it camel case to match everything else in Node. But the person who wrote it has left, so I'm not 100% sure. Let's see if there is a bread crumb trail...
...Unfortunately no. It looks like it was one big commit without further explanation.