Adds Storage Veneer samples and CLI #173
Conversation
|
NOTE: These samples will need to be updated for googleapis/google-cloud-php#169 once a new tag is available. |
storage/api/src/AclCommand.php
Outdated
| $bucketName = $input->getArgument('bucket'); | ||
| $entity = $input->getOption('entity'); | ||
| $role = $input->getOption('role'); | ||
| if ($objectName = $input->getArgument('object')) { |
There was a problem hiding this comment.
The if-else clauses here make me nervous.
Can it be simpler? How about to split this command into 3 or 4 commands like object-acl, default-acl, show-acl?
There was a problem hiding this comment.
How about bucket-acl and object-acl?
| 'Supply your encryption key' | ||
| ) | ||
| ->addOption( | ||
| 'generate', |
There was a problem hiding this comment.
Nit: So it's encryption --generate. I would say --generate-key
| /** | ||
| * For instructions on how to run the full sample: | ||
| * | ||
| * @see https://github.com/GoogleCloudPlatform/php-docs-samples/tree/master/storage/api/README.md |
There was a problem hiding this comment.
Does the README really have the instruction? How about to point them to our docs?
There was a problem hiding this comment.
The docs don't show how to run the CLI
There was a problem hiding this comment.
So how about the README? It's now almost empty.
|
|
||
| use Exception; | ||
|
|
||
| # [START rotate_encryption_key] |
There was a problem hiding this comment.
Do you want to put this snippet on our docs?
There was a problem hiding this comment.
We might as well leave it in - it'll be supported by the veneers soon.
| ], | ||
| ['interactive' => false] | ||
| ); | ||
| $this->expectOutputRegex("/Deleted allAuthenticatedUsers from \S+ ACL/"); |
There was a problem hiding this comment.
This test will pass when the API call silently fails. Ideally can we test the actual behavior? Maybe you can file a bug for it and tackle it later.
There was a problem hiding this comment.
This is now fixed!
There was a problem hiding this comment.
Hmm, it's not. I mean, for example, if the following call:
$acl->add($entity, $role, $options);doesn't raise any exception, but it fails to actually add the acl for some reason, our test will pass, but it's not working.
I think the ideal test should check if the acl is actually created as expected.
I'm fine with the current test for the initial version though.
There was a problem hiding this comment.
If the API call to $acl->add fails, it will throw an exception. These tests are not to test the underlying google-cloud library, but rather to test the samples. I believe the tests are sufficient.
There was a problem hiding this comment.
If it fails with an exception, yes you're right. But potentially it can fail without throwing any exceptions. For example, the code may put the argument in a wrong order, normally it will throw an exception, but potentially they are legal, etc.
Anyway, you can merge and discuss this outside of this PR
| ], | ||
| ['interactive' => false] | ||
| ); | ||
| $this->expectOutputRegex("/Deleted \S+$basename-copy/"); |
There was a problem hiding this comment.
Be careful about calling expectOutputRegex multiple times. Only the last call will take effect.
There was a problem hiding this comment.
Good catch, thanks. These are now all tackled in a single assert.
|
LGTM if you want to merge this with following TODOs:
|
2 participants
No description provided.