PHP Bucketlock samples and tests #692

billyjacobson · 2018-08-27T19:35:25Z

frankyn

A few nits, but overall lgtm! Thanks @billyjacobson!

frankyn · 2018-08-27T21:08:21Z

storage/src/remove_retention_policy.php

+    $bucket = $storage->bucket($bucketName);
+    $bucket->update([
+        'retentionPolicy' => [
+        ]]);


Could you move up the closing bracket?

frankyn · 2018-08-27T22:31:57Z

storage/storage.php

    });

+
+// Create Buckets command


I'm guessing this was copied over from another file.

frankyn · 2018-08-27T22:32:29Z

storage/storage.php

+
+// Create Buckets command
+$application->add(new Command('bucketlock'))
+    ->setDescription('Manage Cloud Storage retention policies')


Manage Cloud Storage object retention.

Generalizing for buckets/objects. Update to Manage Cloud Storage retention policies and holds

Should the entire command set be labeled "retention-policy" then? or is it fine to keep it as bucket-lock

bucketlock is okay.

storage/test/BucketLockCommandTest.php

frankyn · 2018-08-27T22:55:59Z

storage/storage.php

+    )
+    ->addArgument('bucket', InputArgument::REQUIRED, 'The Cloud Storage bucket name')
+    ->addArgument('object', InputArgument::OPTIONAL, 'The Cloud Storage object name')
+    ->addArgument('retention-period', InputArgument::OPTIONAL, 'The length of the retention period')


suffix it with " in seconds"

storage/src/remove_retention_policy.php

Adding conditional for removing retention policy

frankyn

LGTM

bshaffer

Fixed some code styles but otherwise this is great once the tests pass!

frankyn

Small nits

storage/test/BucketLockCommandTest.php

storage/README.md

frankyn · 2018-10-04T18:36:59Z

@grayside could you help review these PHP samples?

frankyn · 2018-10-04T18:37:53Z

@grayside there is also version difficulties with the composer.lock file. We aren't sure how to fix this issue. Any tips would be awesome.

frankyn · 2018-10-04T19:52:43Z

cc: @chingor13 will help review.

chingor13

@return void should be removed from the phpdoc. There's no void type in PHP.
It's unlikely users will want to call update without checking the return value

storage/src/enable_default_event_based_hold.php

storage/src/lock_retention_policy.php

storage/src/get_default_event_based_hold.php

storage/src/get_retention_policy.php

chingor13 · 2018-10-04T20:26:18Z

storage/src/lock_retention_policy.php

+ *
+ * @param string $bucketName the name of your Cloud Storage bucket.
+ *
+ * @return void


We can skip @return void

storage/src/release_event_based_hold.php

storage/src/release_temporary_hold.php

chingor13 · 2018-10-04T20:28:52Z

storage/src/remove_retention_policy.php

+    $bucket = $storage->bucket($bucketName);
+    $bucket->reload();
+
+    if ($bucket->info()['retentionPolicy']['isLocked']) {


Do we not surface these attributes in a cleaner way? If they're well known, we should probably consider trying to expose them in a better way than nested associative arrays.

It could be a feature request at this point. It currently follows how metadata is handled by the library.

frankyn · 2018-10-04T21:39:18Z

@chingor13 do you have input on how to fix the composer.lock? There's a mismatch of versions.

chingor13 · 2018-10-04T22:02:52Z

@frankyn It looks like storage/composer.lock was removed upstream causing the conflict. composer can resolve the dependencies from scratch without it.

frankyn · 2018-10-05T16:55:05Z

Something is up with travis.

frankyn · 2018-10-05T17:41:56Z

Chatted with @bshaffer, looks like Travis CI requirement is a bug. He was trying to remove Travis CI but he's no longer able to remove it (I tried as well). We are technically blocked until this is resolved.

frankyn · 2018-10-05T17:45:52Z

Nvm, unblocked.

PHP Bucketlock samples and tests

b351aa5

billyjacobson added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 27, 2018

billyjacobson requested review from bshaffer and frankyn August 27, 2018 19:36

frankyn suggested changes Aug 27, 2018

View reviewed changes

frankyn reviewed Aug 27, 2018

View reviewed changes

storage/src/remove_retention_policy.php Show resolved Hide resolved

Cleaning up samples

790a526

Adding conditional for removing retention policy

frankyn approved these changes Aug 28, 2018

View reviewed changes

billyjacobson and others added 2 commits August 31, 2018 12:29

Updating readme and changing bucketlock comamnd to be bucket-lock

69931b2

fixes cs for BucketLockCommandTest

95fba1a

bshaffer approved these changes Sep 20, 2018

View reviewed changes

billyjacobson added 5 commits October 2, 2018 11:07

Adding additional functions for bucket lock samples

abfc6b8

Updating composer.lock

a6e8539

Downgrading PHP version

6a67db5

trigger

544beed

trigger

f5e78a5

frankyn added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Oct 4, 2018

kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Oct 4, 2018

frankyn requested a review from grayside October 4, 2018 17:46

frankyn suggested changes Oct 4, 2018

View reviewed changes

storage/test/BucketLockCommandTest.php Outdated Show resolved Hide resolved

storage/README.md Outdated Show resolved Hide resolved

billyjacobson assigned billyjacobson and unassigned billyjacobson Oct 4, 2018

frankyn removed the request for review from grayside October 4, 2018 19:52

chingor13 requested changes Oct 4, 2018

View reviewed changes

Remove composer.lock

fcef8da

frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Oct 5, 2018

billyjacobson added 2 commits October 5, 2018 11:12

Renaming event based to event-based

8b8ae45

Removing try/catches for the updates

d2fb8bf

billyjacobson force-pushed the bucketlock branch from 79e4a42 to d2fb8bf Compare October 5, 2018 16:30

frankyn approved these changes Oct 5, 2018

View reviewed changes

chingor13 approved these changes Oct 5, 2018

View reviewed changes

frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Oct 5, 2018

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2018

billyjacobson added 2 commits October 5, 2018 16:09

Fix not found in array errors for tests

18fe8bc

More random bucket names

3e7ef05

frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2018

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2018

Add sleep to avoid the rate limit for creating/deleting.

18f166e

frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 5, 2018

frankyn merged commit cc58f22 into master Oct 5, 2018

frankyn deleted the bucketlock branch October 5, 2018 21:07

PHP Bucketlock samples and tests #692

PHP Bucketlock samples and tests #692

Uh oh!

Conversation

billyjacobson commented Aug 27, 2018 • edited by sirtorry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

bshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

frankyn commented Oct 4, 2018

Uh oh!

frankyn commented Oct 4, 2018

Uh oh!

frankyn commented Oct 4, 2018

Uh oh!

chingor13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankyn commented Oct 4, 2018

Uh oh!

chingor13 commented Oct 4, 2018

Uh oh!

frankyn commented Oct 5, 2018

Uh oh!

frankyn commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankyn commented Oct 5, 2018

Uh oh!

Reviewers

billyjacobson commented Aug 27, 2018 •

edited by sirtorry

Loading

frankyn commented Oct 5, 2018 •

edited

Loading