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

fix(Spanner): default creator role is empty str #5941

Merged

Conversation

vishwarajanand
Copy link
Contributor

@vishwarajanand vishwarajanand commented Mar 9, 2023

We need to migrate default creator role to empty string instead of a null string to avoid failures in System\SessionTest::testCacheSessionPool where creator_role is null (not empty string) and emulator is rejecting it:

'creator_role' => isset($this->config['databaseRole']) ? $this->config['databaseRole'] : null

This change will bring spanner's creator roles in line with other language libraries, such as Go and with autogenerated changes:

private $creator_role = '';

Note: unfortunately, this will be a BC breaking change.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Mar 9, 2023
@vishwarajanand vishwarajanand marked this pull request as ready for review March 9, 2023 17:36
@vishwarajanand vishwarajanand requested review from a team as code owners March 9, 2023 17:36
Copy link
Contributor

@ajupazhamayil ajupazhamayil left a comment

Choose a reason for hiding this comment

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

As per the discussion, no defaults mentioned in the proto file here https://github.com/googleapis/googleapis/blob/master/google/spanner/v1/spanner.proto#L362. Seems like the string type in proto automatically assumes empty string as defaults in the gapic files here

private $creator_role = '';

Spanner/src/Database.php Outdated Show resolved Hide resolved
Spanner/src/Batch/BatchClient.php Outdated Show resolved Hide resolved
Spanner/src/Instance.php Outdated Show resolved Hide resolved
Spanner/src/Operation.php Outdated Show resolved Hide resolved
Spanner/src/Session/CacheSessionPool.php Outdated Show resolved Hide resolved
@bshaffer bshaffer enabled auto-merge (squash) March 17, 2023 13:57
@bshaffer bshaffer merged commit f922d6c into googleapis:main Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. breaking change allowed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants