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.

Sign up for GitHub

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: support customer managed instance configurations #5523

Merged
merged 18 commits into from
Oct 14, 2022
Merged

feat: support customer managed instance configurations #5523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3c4275f
feat: support customer managed instance configurations
rahul2393 Sep 29, 2022
80b646f
fix tests
rahul2393 Sep 29, 2022
e137583
add missing funcs
rahul2393 Sep 30, 2022
2dd8290
fix test
rahul2393 Sep 30, 2022
d72866b
fix: address review comments
olavloite Oct 5, 2022
f65ea62
fix: add filter for name in field mask to tests
olavloite Oct 6, 2022
56aa92e
Merge branch 'main' into read-only-replicas
olavloite Oct 6, 2022
68b4301
test: split integration tests
olavloite Oct 6, 2022
9715cc6
Merge branch 'read-only-replicas' of github.com:googleapis/google-clo…
olavloite Oct 6, 2022
4a60e5d
chore: fix style issues
olavloite Oct 6, 2022
0cf42b0
Merge branch 'main' into read-only-replicas
olavloite Oct 6, 2022
e843239
Merge branch 'main' into read-only-replicas
rahul2393 Oct 9, 2022
077f21c
Merge branch 'main' into read-only-replicas
olavloite Oct 12, 2022
3b78a7a
Merge branch 'main' into read-only-replicas
olavloite Oct 12, 2022
91d1c73
misc review adjustments
dwsupplee Oct 14, 2022
80b4e14
drop trailing comma
dwsupplee Oct 14, 2022
236b612
Merge branch 'main' into read-only-replicas
rahul2393 Oct 14, 2022
6879a40
Merge branch 'main' into read-only-replicas
saranshdhingra Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions Spanner/src/Connection/ConnectionInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,26 @@ public function listInstanceConfigs(array $args);
*/
public function getInstanceConfig(array $args);

/**
* @param array $args
*/
public function createInstanceConfig(array $args);

/**
* @param array $args
*/
public function updateInstanceConfig(array $args);

/**
* @param array $args
*/
public function deleteInstanceConfig(array $args);

/**
* @param array $args
*/
public function listInstanceConfigOperations(array $args);

/**
* @param array $args
*/
Expand Down
131 changes: 129 additions & 2 deletions Spanner/src/Connection/Grpc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@
use Google\Cloud\Spanner\Admin\Database\V1\RestoreDatabaseEncryptionConfig;
use Google\Cloud\Spanner\Admin\Database\V1\RestoreDatabaseMetadata;
use Google\Cloud\Spanner\Admin\Database\V1\UpdateDatabaseDdlMetadata;
use Google\Cloud\Spanner\Admin\Instance\V1\CreateInstanceConfigMetadata;
use Google\Cloud\Spanner\Admin\Instance\V1\CreateInstanceMetadata;
use Google\Cloud\Spanner\Admin\Instance\V1\Instance;
use Google\Cloud\Spanner\Admin\Instance\V1\InstanceAdminClient;
use Google\Cloud\Spanner\Admin\Instance\V1\InstanceConfig;
use Google\Cloud\Spanner\Admin\Instance\V1\UpdateInstanceConfigMetadata;
use Google\Cloud\Spanner\Admin\Instance\V1\UpdateInstanceMetadata;
use Google\Cloud\Spanner\Operation;
use Google\Cloud\Spanner\SpannerClient as ManualSpannerClient;
Expand Down Expand Up @@ -131,6 +134,14 @@ class Grpc implements ConnectionInterface
'method' => 'createDatabase',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.database.v1.CreateDatabaseMetadata',
'message' => CreateDatabaseMetadata::class
], [
'method' => 'createInstanceConfig',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.instance.v1.CreateInstanceConfigMetadata',
'message' => CreateInstanceConfigMetadata::class
], [
'method' => 'updateInstanceConfig',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.instance.v1.UpdateInstanceConfigMetadata',
'message' => UpdateInstanceConfigMetadata::class
], [
'method' => 'createInstance',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.instance.v1.CreateInstanceMetadata',
Expand Down Expand Up @@ -163,6 +174,14 @@ class Grpc implements ConnectionInterface
'method' => 'createDatabase',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.database.v1.Database',
'message' => Database::class
], [
'method' => 'createInstanceConfig',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.instance.v1.InstanceConfig',
'message' => InstanceConfig::class
], [
'method' => 'updateInstanceConfig',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.instance.v1.InstanceConfig',
'message' => InstanceConfig::class
], [
'method' => 'createInstance',
'typeUrl' => 'type.googleapis.com/google.spanner.admin.instance.v1.Instance',
Expand Down Expand Up @@ -275,6 +294,74 @@ public function getInstanceConfig(array $args)
]);
}

/**
* @param array $args
*/
public function createInstanceConfig(array $args)
{
$instanceConfigName = $args['name'];

$instanceConfig = $this->instanceConfigObject($args, true);
$res = $this->send([$this->getInstanceAdminClient(), 'createInstanceConfig'], [
$this->pluck('projectName', $args),
$this->pluck('instanceConfigId', $args),
$instanceConfig,
$this->addResourcePrefixHeader($args, $instanceConfigName)
]);

return $this->operationToArray($res, $this->serializer, $this->lroResponseMappers);
}

/**
* @param array $args
*/
public function updateInstanceConfig(array $args)
{
$instanceConfigName = $args['name'];

$instanceConfigArray = $this->instanceConfigArray($args);

$fieldMask = $this->fieldMask($instanceConfigArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I found in local testing, I think the name key needs to be removed before sending it to the fieldMask method.
Let me know if you think that's not the case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right. I've changed that.

Are there any plans for running the integration tests on real Cloud Spanner for PRs in this repository? Or is there any way that they can be kicked off manually? I guess that would have caught this earlier, as it is probably a case of making a small change after having executed all the integration tests, and then forgetting to run them all once more after this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

The integration tests run as a continuous job but not on PRs.

You can test them manually by running phpunit -c phpunit-system.xml.dist --verbose tests/System/AdminTest.

Optionally you could use the --filter argument if you don't want other tests to run. It accepts a regex.


$instanceConfigObject = $this->serializer->decodeMessage(new InstanceConfig(), $instanceConfigArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use $this->instanceConfigObject($args, true); as we have done in the createInstanceConfig method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because:

  1. We need to call $instanceConfigArray = $this->instanceConfigArray($args); to get the instance config as an array, as that is needed as the input for getting the field mask.
  2. Calling instanceConfigArray($args) changes the $args array, so it cannot be used again for a call to $this->instanceConfigObject(..);

This function is following the same structure as the existing method for updateInstance and reusing the existing functions from that as much as possible.


$res = $this->send([$this->getInstanceAdminClient(), 'updateInstanceConfig'], [
$instanceConfigObject,
$fieldMask,
$this->addResourcePrefixHeader($args, $instanceConfigName)
]);

return $this->operationToArray($res, $this->serializer, $this->lroResponseMappers);
}

/**
* @param array $args
*/
public function deleteInstanceConfig(array $args)
{
$instanceConfigName = $this->pluck('name', $args);
return $this->send([$this->getInstanceAdminClient(), 'deleteInstanceConfig'], [
$instanceConfigName,
$this->addResourcePrefixHeader($args, $instanceConfigName)
]);
}

/**
* @param array $args
*/
public function listInstanceConfigOperations(array $args)
{
$projectName = $this->pluck('projectName', $args);
$result = $this->send([$this->getInstanceAdminClient(), 'listInstanceConfigOperations'], [
$projectName,
$this->addResourcePrefixHeader($args, $projectName)
]);
foreach ($result['operations'] as $index => $operation) {
$result['operations'][$index] = $this->deserializeOperationArray($operation);
}
return $result;
}

/**
* @param array $args
*/
Expand Down Expand Up @@ -1227,6 +1314,45 @@ private function createTransactionSelector(array &$args)
return $selector;
}

/**
* Converts a PHP array to an InstanceConfig proto message.
*
* @param array $args
saranshdhingra marked this conversation as resolved.
Show resolved Hide resolved
* @param bool $required
* @return InstanceConfig
*/
private function instanceConfigObject(array &$args, $required = false)
{
return $this->serializer->decodeMessage(
new InstanceConfig(),
$this->instanceConfigArray($args, $required)
);
}

/**
* Creates a PHP array with only the fields that are relevant for an InstanceConfig.
*
* @param array $args
saranshdhingra marked this conversation as resolved.
Show resolved Hide resolved
* @param bool $required
* @return array
*/
private function instanceConfigArray(array &$args, $required = false)
{
$argsCopy = $args;
return array_intersect_key([
'name' => $this->pluck('name', $args, $required),
'baseConfig' => $this->pluck('baseConfig', $args, $required),
'displayName' => $this->pluck('displayName', $args, $required),
'configType' => $this->pluck('configType', $args, $required),
'replicas' => $this->pluck('replicas', $args, $required),
'optionalReplicas' => $this->pluck('optionalReplicas', $args, $required),
'leaderOptions' => $this->pluck('leaderOptions', $args, $required),
'reconciling' => $this->pluck('reconciling', $args, $required),
'state' => $this->pluck('state', $args, $required),
'labels' => $this->pluck('labels', $args, $required),
], $argsCopy);
}

/**
* @param array $args
* @param bool $required
Expand Down Expand Up @@ -1276,7 +1402,9 @@ private function fieldMask($instanceArray)
{
$mask = [];
foreach (array_keys($instanceArray) as $key) {
$mask[] = Serializer::toSnakeCase($key);
if ($key !== 'name') {
$mask[] = Serializer::toSnakeCase($key);
}
}
return $this->serializer->decodeMessage(new FieldMask(), ['paths' => $mask]);
}
Expand Down Expand Up @@ -1385,7 +1513,6 @@ private function getInstanceAdminClient()
return $this->instanceAdminClient;
}
//@codeCoverageIgnoreEnd

$this->instanceAdminClient = $this->constructGapic(InstanceAdminClient::class, $this->grpcConfig);

return $this->instanceAdminClient;
Expand Down
Loading