feat(spanner): add admin instance wrapper #16080

bajajneha27 · 2021-11-19T10:06:32Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea.
Follow the instructions in CONTRIBUTING. Most importantly, ensure the tests and linter pass by running bundle exec rake ci in the gem subdirectory.
Update code documentation if necessary.

hengfengli · 2021-11-20T00:05:53Z

@bajajneha27 Can you please fix the tests before we can review this PR? Thanks.

bajajneha27 · 2021-11-22T04:43:28Z

@bajajneha27 Can you please fix the tests before we can review this PR? Thanks.

Yeah, I'm working on it. I'll update once done. Thanks.

bajajneha27 · 2021-11-22T12:47:20Z

Hi @hengfengli , does emulator support update instance request? I'm getting the following error here:

Google::Cloud::UnimplementedError: 12:Cloud Spanner Emulator does not support updating instances

hengfengli · 2021-11-22T20:54:18Z

Hi @hengfengli , does emulator support update instance request? I'm getting the following error here:
Google::Cloud::UnimplementedError: 12:Cloud Spanner Emulator does not support updating instances

No I don't think it supports that. You have to skip the test just for the emulator.

skuruppu · 2021-11-24T04:05:45Z

google-cloud-spanner/test/google/cloud/spanner/admin/instance/client_test.rb

+      Gapic::ServiceStub.stub :new, :stub do
+          client = Google::Cloud::Spanner::Admin::Instance.instance_admin project_id: "1234"
+          assert_kind_of Google::Cloud::Spanner::Admin::Instance::V1::InstanceAdmin::Client, client
+          assert_equal emulator_host, client.configure.endpoint


In #13456, I was also trying to assert if the credentials are :this_channel_is_insecure. But I couldn't get it working. Would you be able to figure out how to get it working?

Thanks Shanika for pointing this out. The credentials is actually set to :this_channel_is_insecure at L105 but then it gets changed over here
I'm not sure about the right behaviour here. Can you please help me out?

@bajajneha27 @skuruppu Unfortunately the actual GRPC::Core::Channel object does not expose the provided credentials, afaik. However, you can use stub to intercept and test the arguments provided to GRPC::Core::Channel#new. I will provide a GH suggestion.

See suggested change, above.

Thank you @quartzmo

hengfengli

LGTM.

quartzmo · 2021-11-24T23:23:28Z

google-cloud-spanner/test/google/cloud/spanner/admin/instance/client_test.rb

+require "gapic/grpc"
+
+class Google::Cloud::Spanner::AdminInstanceClientConstructionMinitest < Minitest::Test
+  def test_instance_admin


Suggested change

def test_instance_admin

def test_instance_admin

emulator_host = "localhost:4567"

channel_args = { "grpc.service_config_disable_resolution" => 1 }

channel_creds = :this_channel_is_insecure

channel = GRPC::Core::Channel.new emulator_host, channel_args, channel_creds

emulator_check = ->(name) { (name == "SPANNER_EMULATOR_HOST") ? emulator_host : nil }

channel_check = ->(chan_host, chan_args, chan_creds) do

assert_equal emulator_host, chan_host

assert_equal channel_args, chan_args

assert_equal channel_creds, chan_creds

channel

end

# Clear all environment variables, except SPANNER_EMULATOR_HOST

ENV.stub :[], emulator_check do

Gapic::ServiceStub.stub :new, :stub do

GRPC::Core::Channel.stub :new, channel_check do

client = Google::Cloud::Spanner::Admin::Instance.instance_admin project_id: "1234"

assert_kind_of Google::Cloud::Spanner::Admin::Instance::V1::InstanceAdmin::Client, client

assert_equal emulator_host, client.configure.endpoint

end

end

end

end

quartzmo

LGTM. I added a suggested change in response to @skuruppu's comment on client_test.rb:30.

…test case.

🤖 I have created a release \*beep\* \*boop\* --- ### 2.11.0 / 2021-12-10 #### Features * add admin instance wrapper ([#16080](https://www.github.com/googleapis/google-cloud-ruby/issues/16080)) * Updated benchwrapper and proto for spanner. ([#16152](https://www.github.com/googleapis/google-cloud-ruby/issues/16152)) * use gRPC clients for instance/database management ([#13157](https://www.github.com/googleapis/google-cloud-ruby/issues/13157)) * wrapper to create generated admin database client. ([#13456](https://www.github.com/googleapis/google-cloud-ruby/issues/13456)) --- ### Commits since last release: * [feat(spanner): Updated benchwrapper and proto for spanner. (#16152)](c67d621) * [feat(spanner): use gRPC clients for instance/database management (#13157)](b6fdf2f) * [feat(spanner): add admin instance wrapper (#16080)](d26c9c9) * [feat(spanner): wrapper to create generated admin database client. (#13456)](24bf835) ### Files edited since last release: <pre><code>google-cloud-spanner/benchmark/benchwrapper/benchwrapper.rb google-cloud-spanner/benchmark/benchwrapper/spanner.proto google-cloud-spanner/benchmark/benchwrapper/spanner_pb.rb google-cloud-spanner/benchmark/benchwrapper/spanner_services_pb.rb google-cloud-spanner/OVERVIEW.md google-cloud-spanner/README.md google-cloud-spanner/lib/google/cloud/spanner/backup.rb google-cloud-spanner/lib/google/cloud/spanner/backup/job.rb google-cloud-spanner/lib/google/cloud/spanner/backup/job/list.rb google-cloud-spanner/lib/google/cloud/spanner/backup/list.rb google-cloud-spanner/lib/google/cloud/spanner/backup/restore/job.rb google-cloud-spanner/lib/google/cloud/spanner/database.rb google-cloud-spanner/lib/google/cloud/spanner/database/backup_info.rb google-cloud-spanner/lib/google/cloud/spanner/database/job.rb google-cloud-spanner/lib/google/cloud/spanner/database/job/list.rb google-cloud-spanner/lib/google/cloud/spanner/database/list.rb google-cloud-spanner/lib/google/cloud/spanner/database/restore_info.rb google-cloud-spanner/lib/google/cloud/spanner/instance.rb google-cloud-spanner/lib/google/cloud/spanner/instance/config.rb google-cloud-spanner/lib/google/cloud/spanner/instance/config/list.rb google-cloud-spanner/lib/google/cloud/spanner/instance/job.rb google-cloud-spanner/lib/google/cloud/spanner/instance/list.rb google-cloud-spanner/lib/google/cloud/spanner/project.rb google-cloud-spanner/acceptance/instance_client_test.rb google-cloud-spanner/lib/google/cloud/spanner/admin/instance.rb google-cloud-spanner/test/google/cloud/spanner/admin/instance/client_config_test.rb google-cloud-spanner/test/google/cloud/spanner/admin/instance/client_test.rb google-cloud-spanner/acceptance/spanner/database_client_test.rb google-cloud-spanner/lib/google/cloud/spanner/admin/database.rb google-cloud-spanner/test/google/cloud/spanner/admin/database/client_config_test.rb google-cloud-spanner/test/google/cloud/spanner/database/client_test.rb </code></pre> [Compare Changes](8b1d497...HEAD) This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

bajajneha27 requested review from a team as code owners November 19, 2021 10:06

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 19, 2021

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Nov 19, 2021

bajajneha27 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 19, 2021

skuruppu approved these changes Nov 24, 2021

View reviewed changes

skuruppu requested review from hengfengli, quartzmo and dazuma November 24, 2021 04:11

hengfengli approved these changes Nov 24, 2021

View reviewed changes

bajajneha27 force-pushed the spanner/admin-instance-wrapper branch from 327ad09 to 06f9a16 Compare November 24, 2021 11:21

quartzmo reviewed Nov 24, 2021

View reviewed changes

quartzmo approved these changes Nov 24, 2021

View reviewed changes

skuruppu mentioned this pull request Nov 25, 2021

feat(spanner): wrapper to create generated admin database client. #13456

Merged

bajajneha27 added 6 commits November 25, 2021 16:55

feat(spanner): Instance admin wrapper.

16568b1

adding test cases.

b8605f8

Adding admin instance client test.

9bf9c69

Require gapic/config for configuration.

709a8fe

Fxing the acceptance test case failure. Skipping the update_instance …

61b46c0

…test case.

test for credentials too.

e659113

bajajneha27 force-pushed the spanner/admin-instance-wrapper branch from 06f9a16 to e659113 Compare November 25, 2021 11:47

bajajneha27 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 25, 2021

bajajneha27 merged commit d26c9c9 into googleapis:main Nov 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner): add admin instance wrapper #16080

feat(spanner): add admin instance wrapper #16080

bajajneha27 commented Nov 19, 2021

hengfengli commented Nov 20, 2021

bajajneha27 commented Nov 22, 2021

bajajneha27 commented Nov 22, 2021

hengfengli commented Nov 22, 2021

skuruppu Nov 24, 2021

bajajneha27 Nov 24, 2021

quartzmo Nov 24, 2021

quartzmo Nov 24, 2021

bajajneha27 Nov 25, 2021

hengfengli left a comment

quartzmo Nov 24, 2021

quartzmo left a comment

-  def test_instance_admin
+  def test_instance_admin
+    emulator_host = "localhost:4567"
+    channel_args = { "grpc.service_config_disable_resolution" => 1 }
+    channel_creds = :this_channel_is_insecure
+    channel = GRPC::Core::Channel.new emulator_host, channel_args, channel_creds
+    emulator_check = ->(name) { (name == "SPANNER_EMULATOR_HOST") ? emulator_host : nil }
+    channel_check = ->(chan_host, chan_args, chan_creds) do
+      assert_equal emulator_host, chan_host
+      assert_equal channel_args, chan_args
+      assert_equal channel_creds, chan_creds
+      channel
+    end
+    # Clear all environment variables, except SPANNER_EMULATOR_HOST
+    ENV.stub :[], emulator_check do
+      Gapic::ServiceStub.stub :new, :stub do
+        GRPC::Core::Channel.stub :new, channel_check do
+          client = Google::Cloud::Spanner::Admin::Instance.instance_admin project_id: "1234"
+          assert_kind_of Google::Cloud::Spanner::Admin::Instance::V1::InstanceAdmin::Client, client
+          assert_equal emulator_host, client.configure.endpoint
+        end
+      end
+    end
+  end

feat(spanner): add admin instance wrapper #16080

feat(spanner): add admin instance wrapper #16080

Conversation

bajajneha27 commented Nov 19, 2021

hengfengli commented Nov 20, 2021

bajajneha27 commented Nov 22, 2021

bajajneha27 commented Nov 22, 2021

hengfengli commented Nov 22, 2021

skuruppu Nov 24, 2021

Choose a reason for hiding this comment

bajajneha27 Nov 24, 2021

Choose a reason for hiding this comment

quartzmo Nov 24, 2021

Choose a reason for hiding this comment

quartzmo Nov 24, 2021

Choose a reason for hiding this comment

bajajneha27 Nov 25, 2021

Choose a reason for hiding this comment

hengfengli left a comment

Choose a reason for hiding this comment

quartzmo Nov 24, 2021

Choose a reason for hiding this comment

quartzmo left a comment

Choose a reason for hiding this comment