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

Jump to bottom

samples(spanner): update samples to use wrappers #890

Merged
merged 61 commits into from
Dec 29, 2021
Merged

samples(spanner): update samples to use wrappers #890

merged 61 commits into from
Dec 29, 2021

Conversation

NivedhaSenthil
Copy link
Member

@NivedhaSenthil NivedhaSenthil commented Dec 6, 2021 •
edited
Loading

  • use the new wrapper instead of initialising the generated client directly

Checklist

  • Tests pass
  • Lint pass: bundle exec rubocop

Closes #891

@NivedhaSenthil
update samples to use wrappers
a138b8d 
- use the new wrapper instead of initialising the generated client directly
@NivedhaSenthil NivedhaSenthil requested a review from a team as a code owner December 6, 2021 11:03
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 6, 2021
@product-auto-label product-auto-label bot added api: storage Issues related to the Cloud Storage API. samples Issues that are directly related to samples. labels Dec 6, 2021
@NivedhaSenthil NivedhaSenthil added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed samples Issues that are directly related to samples. labels Dec 6, 2021
@NivedhaSenthil NivedhaSenthil changed the title samples(storage): update samples to use wrappers samples(spanner): update samples to use wrappers Dec 6, 2021
@NivedhaSenthil NivedhaSenthil added api: spanner Issues related to the Spanner API. and removed api: storage Issues related to the Cloud Storage API. labels Dec 6, 2021
@NivedhaSenthil
update spanner_create_instance
81e5e2b 
- use the new wrapper instead
@hengfengli
Copy link
Contributor

@NivedhaSenthil Can you please fix the CI errors? Also, why do we put a Do Not Merge label for this PR?

@NivedhaSenthil
Copy link
Member Author

@hengfengli Thanks for taking a look at this... the changes for the admin wrapper is not released yet.. hence wanted to wait till it is released. Also the test failures are due to the same reason as it is using the latest released version of google-cloud-spanner library which don't have the changes yet.

Created a PR to track on the completed samples and get some early feedback.

@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Dec 7, 2021
hengfengli
hengfengli reviewed Dec 8, 2021
View reviewed changes
spanner/spanner_samples.rb
version_retention_period = "7d"
database_admin_client = Google::Cloud::Spanner::Admin::Database.database_admin project_id: project_id

instance_path = database_admin_client.instance_path project: project_id, instance: instance_id
Copy link
Contributor

Choose a reason for hiding this comment

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

So we don't store project_id in database_admin_client, right? We have to pass project_id again here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can remove it from client creation 👍

hengfengli
hengfengli approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

@NivedhaSenthil NivedhaSenthil requested a review from a team December 21, 2021 03:28
@NivedhaSenthil NivedhaSenthil requested review from gcseh and a team as code owners December 21, 2021 05:37
@NivedhaSenthil
Copy link
Member Author

Skipping backup tests due to timeout issue. To be fixed as part of #893

@NivedhaSenthil NivedhaSenthil removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 22, 2021
@NivedhaSenthil
Revert "update to use specific mini_racer version"
133270b 
This reverts commit 2565dc4.
@NivedhaSenthil
Revert "update with key created in us-west1"
666f1e2 
This reverts commit 1c16232.
@NivedhaSenthil
Revert "update jobs api-client"
531f964 
This reverts commit 4594483.
@NivedhaSenthil
Revert "update versions for pubsub"
9a374ff 
This reverts commit e64678e.
@NivedhaSenthil
Revert "update mini_racer"
85e9c73 
This reverts commit 054ea8e.
@NivedhaSenthil
Revert "use mini_racer version less than 0.5.0"
3a424b5 
This reverts commit fe88aab.
@NivedhaSenthil
Revert "check with new instance"
9bd1af4 
This reverts commit 63e032c.
@NivedhaSenthil
Revert "update with instance created in us-west1"
85b8acf 
This reverts commit a3cbabf.
@NivedhaSenthil
Revert "test config change"
82c64ea 
This reverts commit 79fe923.
@NivedhaSenthil
revert test config changes
5160b10
@NivedhaSenthil
revert test config changes
c2675de
@NivedhaSenthil
revert test config changes
eba8ba4
bajajneha27
bajajneha27 approved these changes Dec 29, 2021
View reviewed changes
Copy link
Contributor

@bajajneha27 bajajneha27 left a comment

Choose a reason for hiding this comment

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

LGTM

@NivedhaSenthil NivedhaSenthil merged commit fb69799 into GoogleCloudPlatform:main Dec 29, 2021
@NivedhaSenthil NivedhaSenthil deleted the update-spanner-samples branch December 29, 2021 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajajneha27 bajajneha27 bajajneha27 approved these changes

@hengfengli hengfengli hengfengli approved these changes

@dazuma dazuma dazuma approved these changes

@gcseh gcseh Awaiting requested review from gcseh

Assignees
No one assigned
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

samples(spanner): update samples to use new client wrapper
4 participants
@NivedhaSenthil @hengfengli @dazuma @bajajneha27