fix: Use the universe domain if it is provided by the user by danieljbruce · Pull Request #1563 · googleapis/nodejs-bigtable

danieljbruce · 2025-01-08T19:00:04Z

Summary:

The user should be able to set the universe domain so that outgoing calls work with a different Google Cloud Universe. Right now, if the user specifies a universe domain then it will not get used and the request will just be sent to the default Bigtable endpoint.

Changes:

src/index.ts: The change here is that when a custom url is not provided, but a universe domain is provided then the servicePath will use the universe domain provided. This is done for each Gapic client and ensures requests will be made to the universe domain instead of the default Bigtable service.

system-test/service-path.ts: Two tests are added to ensure the service path is always set correctly. For instance, when a custom endpoint is provided then the service path will still always be that custom endpoint. When a custom endpoint is NOT provided, but a universe domain is provided then the service path is set to use the universe domain instead of the default endpoint.

Alternatives:

Instead of requiring the user to set the universe domain for each Gapic client, we could expose a universeDomain option that would apply to all clients. While this would make things easier for users, it is an API change so it is not reversible. For now we should not change the API since users already have a way to specify the universe domains for the Gapic clients. We can always add this option later.
When a custom endpoint is not provided then in the handwritten layer we could not provide a service path. The Pros of this option is that we can rely on the code in the Gapic layer for building the service path instead of creating the getDomain function introducing duplicate code and this would reduce technical debt. The main problem with this option is that it means providedCustomServicePath will now be undefined and this value is used in the grpc stub so we don't know how such a change might affect the outgoing API calls. The code to implement this alternative also gets messy if Alternative 1 isn't adopted as well.

…ejs-bigtable into universe-domain-2

danieljbruce · 2025-01-10T14:42:57Z

src/index.ts

-        servicePath: customEndpointBaseUrl || defaultAdminBaseUrl,
+        servicePath:
+          customEndpointBaseUrl ||
+          `bigtableadmin.${getDomain(options.BigtableTableAdminClient)}`,


If no universe domain and custom endpoint is provided then this will still be bigtableadmin.googleapis.com.

danieljbruce · 2025-01-10T14:43:57Z

src/index.ts

-        servicePath: customEndpointBaseUrl || defaultBaseUrl,
+        servicePath:
+          customEndpointBaseUrl ||
+          `bigtable.${getDomain(options.BigtableClient)}`,


If no universe domain and custom endpoint is provided then this will still be bigtable.googleapis.com.

daniel-sanche

Mostly LGTM, with a few small comments.

I'm not too familiar with the universe domains yet though, so you might also want to assign a reviewer who understands potential complications there

daniel-sanche · 2025-01-10T19:17:52Z

src/index.ts

+ * @returns {string} The universe domain.
+ */
+function getDomain(opts?: gax.ClientOptions) {
+  // This code for universe domain was taken from the Gapic Layer.


can you add a permalink to where this came from?

daniel-sanche · 2025-01-10T19:20:40Z

src/index.ts

-        servicePath: customEndpointBaseUrl || defaultBaseUrl,
+        servicePath:
+          customEndpointBaseUrl ||
+          `bigtable.${getDomain(options.BigtableClient)}`,


What would you think about adding pathPrefix as an argument to getDomain (and maybe renaming the method if needed), instead of manually building the string here? I think that would be a bit easier to read, and seems more natural to me since the returned string isn't useful on its own

Yup. That would be fine.

bshaffer

This looks good - a few test suggestions and questions, but otherwise LGTM

bshaffer · 2025-01-16T19:22:27Z

src/index.ts

+    opts?.universeDomain ??
+    opts?.universe_domain ??


Is this a nodeJS convention to support both casings? AFAIK this is not the case in any other language implementation

This is code is taken from here. I suggest we leave it as is because this is just Gapic code, but if you have an exact suggestion then maybe I could try that as well?

In the JSON file, it's defined as universe_domain. I don't have a exact suggestion other than to say since the language agnostic spec doesn't require both casings, it must be specific to NodeJS.

According to #1386, that's how support was added for all GAPICs, so LGTM

Ok. Thanks for looking into that.

bshaffer · 2025-01-16T19:24:08Z

testproxy/known_failures.txt

+TestSampleRowKeys_Generic_CloseClient\|
+TestSampleRowKeys_Generic_Headers\|
+TestSampleRowKeys_NoRetry_NoEmptyKey\|
+TestSampleRowKeys_Retry_WithRetryInfo


Why are these part of this PR? These tests shouldn't be affected as a result of these changes

These needed to be added because suddenly conformance tests started failing for sampleRowKeys. This is most likely because new tests were recently added to the test runner.

I'd pull these out of the PR.

These have been pulled out.

bshaffer · 2025-01-16T19:26:37Z

system-test/service-path.ts

+import {BigtableClient, BigtableInstanceAdminClient} from '../src/v2';
+
+describe('Service Path', () => {
+  it('Setting universe domain should set the service path', async () => {


I would suggest also

adding a test for when the universe domain ENV VAR is set.

setting the ENV VAR in the other two tests as well, which will ensure that it's not used when the universeDomain and apiEndpoint options are set.

ok. I'll do that next.

# Conflicts: # testproxy/known_failures.txt Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable into universe-domain-2

danieljbruce added 7 commits January 7, 2025 13:12

Add the universe domain test

975f8c9

Don’t override service path if custom path not set

2b1e7a0

Add universe domain to bigtable options

61183be

Update system tests to look at getRows call

74809ba

Add unit tests to check service path

9041e2e

Update the system test

162da6d

Remove system test changes

3ee3f5b

product-auto-label bot added size: m Pull request size is medium. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Jan 8, 2025

danieljbruce and others added 13 commits January 8, 2025 14:05

Remove only

48b6a63

Put the new tests in a separate file

d5f0ddb

Eliminate new tests from old location

d18d047

Make service path based on universe domain

7b3d50f

Add space back

6c21bc9

Check errors for the timeouts in the test

dfa5af0

Remove service error import

7cef954

Add comments about code reuse

5073624

Remove universe domain option

1871316

Remove only

9dd24d7

Merge branch 'main' into universe-domain-2

b9907e4

Add header

a90740d

Merge branch 'universe-domain-2' of https://github.com/googleapis/nod…

7ec609f

…ejs-bigtable into universe-domain-2

danieljbruce marked this pull request as ready for review January 8, 2025 21:59

danieljbruce requested a review from a team as a code owner January 8, 2025 21:59

danieljbruce requested a review from a team January 8, 2025 21:59

danieljbruce added 3 commits January 9, 2025 15:51

Add known failures to mandatory conformance tests

112f2cf

Add separator

c68d5da

Add another known failure

eff964e

danieljbruce commented Jan 10, 2025

View reviewed changes

danieljbruce requested a review from daniel-sanche January 10, 2025 14:45

daniel-sanche approved these changes Jan 10, 2025

View reviewed changes

danieljbruce requested a review from bshaffer January 10, 2025 21:20

bshaffer reviewed Jan 16, 2025

View reviewed changes

danieljbruce added 4 commits January 21, 2025 16:17

Add permalink

b8215d0

Make getDomain build whole string

3fd01fc

Update from test comments about environment variab

cea3b8d

Merge branch 'main' of https:/

3ac224a

# Conflicts: # testproxy/known_failures.txt Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable into universe-domain-2

danieljbruce requested a review from bshaffer January 22, 2025 15:44

danieljbruce mentioned this pull request Jan 22, 2025

feat: Clients should support the universe domain option googleapis/nodejs-datastore#1365

Closed

bshaffer approved these changes Jan 22, 2025

View reviewed changes

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 22, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 22, 2025

danieljbruce added the automerge Merge the pull request once unit tests and other checks pass. label Jan 22, 2025

gcf-merge-on-green bot merged commit d26ecb8 into main Jan 22, 2025
21 of 25 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 22, 2025

gcf-merge-on-green bot deleted the universe-domain-2 branch January 22, 2025 19:56

release-please bot mentioned this pull request Jan 22, 2025

chore(main): release 6.0.0 #1471

Merged

Conversation

danieljbruce commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieljbruce Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danieljbruce commented Jan 8, 2025 •

edited

Loading

danieljbruce Jan 10, 2025 •

edited

Loading

daniel-sanche Jan 10, 2025 •

edited

Loading