-
Notifications
You must be signed in to change notification settings - Fork 1k
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): add samples for soft-delete and restore buckets #2067
base: main
Are you sure you want to change the base?
Feat(Storage): add samples for soft-delete and restore buckets #2067
Conversation
initial commit for bucket restore samples
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
Hi @bshaffer, this should be ready for review when you get a second! |
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 ran these tests locally and received the following errors:
1) Google\Cloud\Samples\Storage\storageTest::testGetRestoreSoftDeletedBucket
Google\Cloud\Core\Exception\ServiceException: {"error":{"code":403,"message":"[email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).","errors":[{"message":"[email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).","domain":"global","reason":"forbidden"}]}}
vendor/google/cloud-core/src/RequestWrapper.php:434
vendor/google/cloud-core/src/RequestWrapper.php:230
vendor/google/cloud-core/src/RestTrait.php:103
vendor/google/cloud-storage/src/Connection/Rest.php:777
vendor/google/cloud-storage/src/Connection/Rest.php:202
vendor/google/cloud-storage/src/Bucket.php:1234
test/storageTest.php:959
--
There was 1 failure:
1) Google\Cloud\Samples\Storage\storageTest::testListSoftDeletedBuckets
Failed asserting that '' contains "Bucket:".
test/storageTest.php:153
Is it possible I need to enable something on the test project php-docs-samples-kokoro
for these tests to pass?
Could you try giving the storage.legacyBucketOwner permission (roles/storage.legacyBucketOwner)? |
@thiyaguk09 this is not the first test that creates a bucket, so I don't think permissions is an issue here |
@danielduhh Agreed. Permissions aren't the likely issue. I suspect the softDeletePolicy isn't being set correctly during bucket creation. I've manually configured it now. Let's add a @bshaffer Could you check if the buckets created are actually configured with the soft delete policy as intended?
|
@thiyaguk09 the buckets are being created with the soft delete option here: php-docs-samples/storage/test/storageTest.php Lines 952 to 956 in 4ef3f1f
I can dig a little deeper to see if this bucket is being created as expected. |
@thiyaguk09 I figured out the issue. The way you are supplying the soft delete options to $storage = new StorageClient();
$options = ['softDeleted' => true, 'generation' => $generation];
$softDeletedBucket = $storage->bucket($bucketName, options: $options); |
@bshaffer Thank you for your thorough investigation and for identifying the correct method for accessing soft-deleted buckets. Your explanation was very clear. You're right, it appears there was an issue with how reload() was handling the soft delete options. We've implemented the changes you suggested, using |
@thiyaguk09 The changes you made are still not correct, and do not follow the code example I provided. You're providing the // What you have (INCORRECT)
$softDeletedBucket = self::$storage->bucket($bucketName);
$options = ['softDeleted' => true, 'generation' => $generation];
$info = $softDeletedBucket->info($options); // What it should be (CORRECT)
$options = ['softDeleted' => true, 'generation' => $generation];
$softDeletedBucket = self::$storage->bucket($bucketName, options: $options);
$info = $softDeletedBucket->info(); |
I apologize for the oversight. Thank you for pointing that out. I will make the necessary corrections to ensure the |
@thiyaguk09 are the tests passing locally? Is this ready for a re-review? |
Yes, it's passing locally and ready for re-review. |
@bshaffer just a friendly nudge to check out this PR when you get a chance. |
Added Storage Samples: