-
Notifications
You must be signed in to change notification settings - Fork 441
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!: Upgrade Bigtable to V2 #7317
Conversation
For the
|
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.
For the failing tests:
See the GAPIC generator: the logic that adds the emulator environment variable is now handed by GAPIC
you don't need to call modifyClientOptions
anymore, you just need to add the spanner client to the EmulatorSupportGenerator
, and you can remove SpannerEmulatorTrait
.
To fix the unit tests, just add the same logic to bypass finals to the bootstrap file in Core/unit-bootstrap.php
, which is the bootstrap file used in the root phpunit.xml.dist
googleapis/gapic-generator-php#717 is needed for emulator tests to pass. |
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.
We need to take a look at Bigtable/owlbot.py
because some very strange things are happening there. I am hoping that the following likes can be removed entirely:
google-cloud-php/Bigtable/owlbot.py
Lines 61 to 117 in 7e98d79
# fix unit test namespace | |
s.replace( | |
'tests/Unit/Admin/*/*.php', | |
r'namespace Google\\Cloud\\Bigtable\\Admin\\Tests\\Unit', | |
r'namespace Google\\Cloud\\Bigtable\\Tests\\Unit\\Admin') | |
# fix test group | |
s.replace( | |
'tests/**/Admin/**/*Test.php', | |
'@group admin', | |
'@group bigtable' + '\n' + ' * bigtable-admin') | |
# Fix class references in gapic samples | |
for version in ['V2']: | |
pathExpr = [ | |
'src/' + version + '/Gapic/BigtableGapicClient.php', | |
'src/Admin/' + version + '/Gapic/*GapicClient.php' | |
] | |
types = { | |
'new BigtableClient': r'new Google\\Cloud\\Bigtable\\' + version + r'\\BigtableClient', | |
'new BigtableInstanceAdminClient': r'new Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\BigtableInstanceAdminClient', | |
r'\$instance = new Instance': r'$instance = new Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\Instance', | |
'= Type::': r'= Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\Instance\\Type::', | |
'new FieldMask': r'new Google\\Protobuf\\FieldMask', | |
r'\$cluster = new Cluster': r'$cluster = new Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\Cluster', | |
'new AppProfile': r'new Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\AppProfile', | |
'new Policy': r'new Google\\Cloud\\Iam\\V1\\Policy', | |
'new BigtableTableAdminClient': r'new Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\BigtableTableAdminClient', | |
'new Table': r'new Google\\Cloud\\Bigtable\\Admin\\' + version + r'\\Table', | |
} | |
for search, replace in types.items(): | |
s.replace( | |
pathExpr, | |
search, | |
replace | |
) | |
### [START] protoc backwards compatibility fixes | |
# roll back to private properties. | |
s.replace( | |
"src/**/V*/**/*.php", | |
r"Generated from protobuf field ([^\n]{0,})\n\s{5}\*/\n\s{4}protected \$", | |
r"""Generated from protobuf field \1 | |
*/ | |
private $""") | |
# Replace "Unwrapped" with "Value" for method names. | |
s.replace( | |
"src/**/V*/**/*.php", | |
r"public function ([s|g]\w{3,})Unwrapped", | |
r"public function \1Value" | |
) | |
### [END] protoc backwards compatibility fixes |
Also, we'll want to make sure googleapis/gapic-generator-php#714 is merged and generated for the Admin
client, as it contains LRO's (cc @noahdietz)
$this->bigtableClient->mutateRows( | ||
Argument::type(MutateRowsRequest::class), | ||
Argument::type('array') | ||
)->shouldBeCalled() |
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 need to check if the OptionalConfiguration
also passed correctly?
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 it seems like we should change this to
$this->bigtableClient->mutateRows( | |
Argument::type(MutateRowsRequest::class), | |
Argument::type('array') | |
)->shouldBeCalled() | |
$this->bigtableClient->mutateRows( | |
Argument::type(MutateRowsRequest::class), | |
$this->options + $options | |
)->shouldBeCalled() |
This will test that the custom options we expect will show up.
I see that you've changed it from key1
=> value1
to transportOptions
, to make sure they're valid call options. That works for me!
After running the tests in
This takes place in the test Fixed in d9c4fd0 |
This PR takes care of the following:
retrySettings.maxRetries
instead of the oldretries
param.Bigtable/owlbot.py
(see chore(Bigtable): generator changes from owlbot for V2 #7352)php-docs-samples/bigtable
pass (see chore: upgrade bigtable samples to v2 GoogleCloudPlatform/php-docs-samples#2012)BREAKING_CHANGE_REASON=RC-1 for the version 2 of the Bigtable library.
BEGIN_VERSION_OVERRIDE
Core: 1.59.0
Bigtable: 2.0.0-RC1
END_VERSION_OVERRIDE