Skip to content
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: Update Emulator configuration function #719

Merged
merged 6 commits into from
May 31, 2024

Conversation

ajupazhamayil
Copy link
Contributor

@ajupazhamayil ajupazhamayil commented May 31, 2024

Resolves #717

BEGIN_COMMIT_OVERRIDE
feat: add emulator support for Bigtable and Spanner clients (#719)
END_COMMIT_OVERRIDE

@ajupazhamayil
Copy link
Contributor Author

Investigating why the code changes didn't update the integration tests goldens!!

@ajupazhamayil
Copy link
Contributor Author

Integration test files are manually updated, introduced some errors to see if the cache delete takes effect. Apparently, the integration tests are running irrespective of the code changes!!

PS: Command used to rotate the cache key: uuidgen | gh secret set CACHE_VERSION -r https://github.com/googleapis/gapic-generator-php

cc: @bshaffer

@ajupazhamayil ajupazhamayil added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 31, 2024
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Only suggestion is we should use the noal-coalescing assignment operator (??=) to ensure that these are defaults that can be overwritten by the user.

See #720 where I've added the operator to the AST

Comment on lines 1057 to 1059
$options['apiEndpoint'] = $emulatorHost;
$options['transportConfig']['grpc']['stubOpts']['credentials'] = ChannelCredentials::createInsecure();
$options['credentials'] = new InsecureCredentialsWrapper();
Copy link
Contributor

@bshaffer bshaffer May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem I see is that where before this was providing default values, it's now overwriting any user-supplied values. What if we use ??= instead?

Suggested change
$options['apiEndpoint'] = $emulatorHost;
$options['transportConfig']['grpc']['stubOpts']['credentials'] = ChannelCredentials::createInsecure();
$options['credentials'] = new InsecureCredentialsWrapper();
$options['apiEndpoint'] ??= $emulatorHost;
$options['transportConfig']['grpc']['stubOpts']['credentials'] ??= ChannelCredentials::createInsecure();
$options['credentials'] ??= new InsecureCredentialsWrapper();

Copy link
Contributor Author

@ajupazhamayil ajupazhamayil Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the customer set the emulator environment variable, endpoint and credentials should be REPLACED to the emulator end point. WDYT?

Ref: https://github.com/googleapis/google-cloud-php/blob/400f247922efc876582a5fb39cf8072e0df63420/Spanner/src/Connection/Grpc.php#L288

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't like that, I think they should just be defaults... I don't ever think an environment variable should REPLACE something in code - they should always function as a default

@bshaffer bshaffer marked this pull request as ready for review May 31, 2024 20:07
@bshaffer bshaffer requested review from a team as code owners May 31, 2024 20:07
@bshaffer bshaffer merged commit 83b772b into main May 31, 2024
4 checks passed
@bshaffer bshaffer deleted the updateEmulatorOptionsFunction branch May 31, 2024 20:08
@bshaffer bshaffer changed the title fix: Update Emulator configuration function feat: Update Emulator configuration function Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kokoro:force-run Add this label to force Kokoro to re-run the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EmulatorSupportGenerator doesn't work for clients when transportConfig is already passed.
2 participants