[Storage] HMAC key samples #946

frankyn · 2019-08-20T05:55:08Z

Integration tests pending adding env var for service account email.
hmac key create, list, get, activate, deactivate, delete samples
lint

bshaffer · 2019-08-20T19:39:13Z

The tests are failing because of the php-cs-fixer command. Download and run it to fix your code styles.

cd storage
php-cs-fixer fix --config=../.php_cs.dist .

frankyn · 2019-08-20T20:25:29Z

Thanks @bshaffer. I also need to add an environment variable into kokoro CI to pass in a service account for tests.

jkwlui · 2019-09-03T22:27:23Z

storage/storage.php

+    ->addArgument('accessId', InputArgument::REQUIRED, 'The Cloud Storage HMAC key access Id')
+    ->addOption('activate', null, InputOption::VALUE_NONE, 'Activate an HMAC key')
+    ->addOption('deactivate', null, InputOption::VALUE_NONE, 'Deactivate an HMAC key')
+    ->addOption('get', null, InputOption::VALUE_NONE, 'Get an HMAC key metadata')


nit: Get an HMAC key's metadata

jkwlui · 2019-09-03T22:30:15Z

storage/test/HmacCommandTest.php

+        $this->assertContains("HMAC key Metadata:", $this->getActualOutput());
+    }
+
+    /** @depends testHmacKeyGet */


This is ok, but ideally each one should test a single command.

jkwlui · 2019-09-03T22:31:51Z

storage/test/HmacCommandTest.php

+        $this->commandTesterCreate = new CommandTester($application->get('hmac-sa-create'));
+        $this->commandTesterManage = new CommandTester($application->get('hmac-sa-manage'));
+        $this->storage = new StorageClient();
+        $this->hmacServiceAccount = self::$projectId . '@appspot.gserviceaccount.com';


We're going to make it configurable via an environment variable, right? Otherwise it conflicts with integration tests.

Tests use a separate project per PHP version.

jkwlui · 2019-09-03T22:33:02Z

storage/src/delete_hmac_key.php

+    $hmacKey = $storage->hmacKey($accessId, $projectId);
+
+    $hmacKey->delete();
+    print("The key is deleted, though it may still appear in StorageClient.hmacKeys() results." . PHP_EOL);


@JesseLovelace brought up a good point. We should indicate that it will show up if showDeletedKeys = true in hmacKeys() results.

jkwlui · 2019-09-09T20:05:51Z

storage/src/delete_hmac_key.php

+
+    $hmacKey->delete();
+    print("The key is deleted, though it may still appear in StorageClient.hmacKeys(\$options=['showDeletedKeys'=>true])");
+    print("results when  is used." . PHP_EOL);


nit: ...results.

dwsupplee

This is looking really nice 👍. Most of my comments are just small quick hits.

storage/src/activate_hmac_key.php

storage/README.md

storage/src/activate_hmac_key.php

dwsupplee · 2019-09-10T22:31:17Z

storage/src/activate_hmac_key.php

+
+    $hmacKey->update('ACTIVE');
+
+    print("The HMAC key is now active." . PHP_EOL);


It's a minor note, but WDYT about using single quotes? It is typically a best practice unless you need string interpolation.

storage/storage.php

dwsupplee · 2019-09-10T23:13:04Z

storage/storage.php

+        } elseif ($input->getOption('delete')) {
+            delete_hmac_key($accessId, $projectId);
+        } else {
+            throw new \Exception('You must provide --activate, --deactivate, --get, or --delete with an HMAC key accessId.');


This goes just a bit over 120 characters, WDYT about?:

throw new \Exception( 'You must provide --activate, --deactivate, --get, or --delete with an HMAC key accessId.' );

storage/test/HmacCommandTest.php

dwsupplee · 2019-09-10T23:17:24Z

storage/test/HmacCommandTest.php

+    public function testHmacKeyList()
+    {
+        $this->commandTesterList->execute(
+          [


The indentation is off a bit here (should be 4 spaces in from the line above) and the closing parenthesis should be on a newline (applies to the rest of the execute commands as well).

I'm following the linter for php-docs-samples. I'm not sure if they follow different specs.

I don't think either of those things will get flagged by the PSR-2 standard used by the linter, however it is fairly common practice to help with readability. The other tests in the suite seem to use the suggested style as well. If you prefer this style, don't consider my suggestion blocking by any means :).

Thanks @dwsupplee I'll move forward for now.

Co-Authored-By: David Supplee <dwsupplee@gmail.com>

frankyn

Updated PTAL, thanks @dwsupplee!

frankyn · 2019-09-10T23:32:58Z

storage/test/HmacCommandTest.php

+    public function testHmacKeyList()
+    {
+        $this->commandTesterList->execute(
+          [


I'm following the linter for php-docs-samples. I'm not sure if they follow different specs.

Add base HMAC key samples

31d4694

frankyn added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 20, 2019

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 20, 2019

Lint

300e0d1

frankyn added 7 commits August 20, 2019 22:36

Fix typo

559af70

Dont redefine projectId

f2b3759

Use specific service account

4783bad

fix missing accessId

03037ad

Fix typo

0c9e24c

Access projectId as static variable

eceb508

Add scope to method call

8a4dcc2

frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 27, 2019

frankyn added 11 commits August 27, 2019 09:30

Remove options typo

407be11

typo in serviceAccountEmail

9f61d2d

Update service account email usage in tests

6dde4e3

Update tests and library version

34061e1

typo in serviceAccountEmail option

2ac043d

Add missing accessId

b7fccb2

Fix sample use of projectId

335981e

Add projectId as a clear parameter

f72586d

Update format

ecad177

Fix tests

f45424f

fix lint issue

8b01905

frankyn requested a review from bshaffer August 28, 2019 16:13

jkwlui reviewed Sep 3, 2019

View reviewed changes

address comments

dc7ea3d

frankyn requested a review from jkwlui September 5, 2019 23:42

Update tests

d41e2de

frankyn added 2 commits September 9, 2019 10:46

Update sample tests

11ed587

Fix lint

3864cb2

frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 9, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 9, 2019

jkwlui reviewed Sep 9, 2019

View reviewed changes

jkwlui approved these changes Sep 9, 2019

View reviewed changes

address feedback

c3d1185

frankyn requested review from dwsupplee and removed request for bshaffer September 9, 2019 20:38

dwsupplee suggested changes Sep 10, 2019

View reviewed changes

frankyn and others added 5 commits September 10, 2019 16:28

Update storage/src/activate_hmac_key.php

bea4dcb

Co-Authored-By: David Supplee <dwsupplee@gmail.com>

Update storage/README.md

a546f37

Co-Authored-By: David Supplee <dwsupplee@gmail.com>

Apply suggestions from code review

7a44547

Co-Authored-By: David Supplee <dwsupplee@gmail.com>

Address comments

6413141

Fix lint

390cfa8

frankyn commented Sep 10, 2019

View reviewed changes

frankyn requested a review from dwsupplee September 10, 2019 23:50

dwsupplee approved these changes Sep 11, 2019

View reviewed changes

frankyn merged commit 3629c3e into master Sep 11, 2019

frankyn deleted the hmac-sa branch September 11, 2019 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage] HMAC key samples #946

[Storage] HMAC key samples #946

frankyn commented Aug 20, 2019 •

edited

bshaffer commented Aug 20, 2019 •

edited

frankyn commented Aug 20, 2019

jkwlui Sep 3, 2019

jkwlui Sep 3, 2019

jkwlui Sep 3, 2019

frankyn Sep 5, 2019 •

edited

jkwlui Sep 3, 2019

jkwlui Sep 9, 2019

dwsupplee left a comment

dwsupplee Sep 10, 2019

dwsupplee Sep 10, 2019

dwsupplee Sep 10, 2019

frankyn Sep 10, 2019

dwsupplee Sep 11, 2019

frankyn Sep 11, 2019

frankyn left a comment

frankyn Sep 10, 2019


		$hmacKey->update('ACTIVE');

		print("The HMAC key is now active." . PHP_EOL);

[Storage] HMAC key samples #946

[Storage] HMAC key samples #946

Conversation

frankyn commented Aug 20, 2019 • edited

bshaffer commented Aug 20, 2019 • edited

frankyn commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn Sep 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsupplee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn commented Aug 20, 2019 •

edited

bshaffer commented Aug 20, 2019 •

edited

frankyn Sep 5, 2019 •

edited