-
Notifications
You must be signed in to change notification settings - Fork 556
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(storage): Soft deleted Bucket Restore #28138
feat(storage): Soft deleted Bucket Restore #28138
Conversation
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.
Mark the PR ready for review once you're ready for it to be reviewed. Also please check the CI job failures.
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
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.
Overall LGTM. Please fix the CI issues.
@@ -223,6 +232,10 @@ def buckets prefix: nil, token: nil, max: nil, user_project: nil | |||
# account, transit costs will be billed to the given project. This | |||
# parameter is required with requester pays-enabled buckets. The | |||
# default is `nil`. | |||
# @param [Integer] generation generation no of bucket |
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.
# @param [Integer] generation generation no of bucket | |
# @param [Integer] generation Generation of the bucket |
Hi @bajajneha27 @JesseLovelace can you please take a look at the sample failure
|
google-cloud-storage/samples/storage_get_soft_deleted_bucket.rb
Outdated
Show resolved
Hide resolved
@shubhangi-google Does the yard / rubocop ci work in your local? |
google-cloud-storage/samples/storage_get_soft_deleted_bucket.rb
Outdated
Show resolved
Hide resolved
Can you please double check that the APIARY i.e. google-apis-storage_v1 has soft delete changes? We might have to bump the storage_v1 version in storage gemspec |
209b570
to
59a3f9c
Compare
updating the version in gemspec did not help will debug this further |
@@ -21,7 +21,7 @@ Gem::Specification.new do |gem| | |||
gem.add_dependency "google-cloud-core", "~> 1.6" | |||
gem.add_dependency "google-apis-core", "~> 0.13" | |||
gem.add_dependency "google-apis-iamcredentials_v1", "~> 0.18" | |||
gem.add_dependency "google-apis-storage_v1", "~> 0.38" | |||
gem.add_dependency "google-apis-storage_v1", "~> 0.40" |
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.
The soft_deleted
field on get_bucket was introduced in storage_v1 0.42 version. Ref
Which version were you using in local?
Also generally, just update to the latest version of the dependency when you do this.
This should solve the issue.
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.
@@ -895,7 +911,7 @@ def encryption_key_headers options, key, copy_source: false | |||
headers = (options[:header] ||= {}) | |||
headers["x-goog-#{source}encryption-algorithm"] = "AES256" | |||
headers["x-goog-#{source}encryption-key"] = Base64.strict_encode64 key | |||
headers["x-goog-#{source}encryption-key-sha256"] = \ |
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.
Any reason why we're modifying this?
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.
rubocop lint issue was coming for this line
@@ -17,6 +17,9 @@ | |||
|
|||
describe "Storage Quickstart" do | |||
let(:project) { Google::Cloud::Storage.new } |
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.
I believe these changes are not needed any more?
soft_deleted= true | ||
|
||
mock = Minitest::Mock.new | ||
mock.expect :list_buckets, list_buckets_gapi(num_buckets,nil,soft_deleted), [project], prefix: nil, page_token: nil, |
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.
nit: instead of passing nil
, we could pass a dummy token here.
Can you please fix the CI failure? |
The CI job is still failing. Can you please take a look? |
Hi @bajajneha27 the ci failure is related to yard and GitHub action which I suppose we don't handle locally |
Yeah, the failure doesn't seem directly related to our changes but we cannot merge the PR because it's one of the required CI jobs. Can you please try and fix it? You should be able to run it locally. |
Add support for restoring soft deleted bucket.
Operation Supported: