Updates KMS samples to use google/cloud-kms client library #800
Conversation
|
Done using the "new" way of writing samples (no "command" class, and no functions surrounding the sample code). Would love some review :) |
| // The resource name of the location associated with the KeyRings | ||
| $parent = sprintf('projects/%s/locations/%s', $projectId, $location); | ||
| // Lists keys in the "global" location. Could also be "us-west1", etc. | ||
| $locationId = 'global'; |
There was a problem hiding this comment.
Why you would ever choose a specific region when global is available?
There was a problem hiding this comment.
Maybe you want to lock down permissions, or maybe it's for organization? I don't know TBH.
There was a problem hiding this comment.
You might choose a region like 'us-east1' if you have specific latency and/or data residency requirements:
https://cloud.google.com/kms/docs/locations#location_types
grayside
left a comment
grayside
left a comment
There was a problem hiding this comment.
Things are looking consistent and aligned with "New New Practices". I raised a couple points that seem worth further discussion, so leaving this at comment for now.
| "files": [ | ||
| "src/functions.php" | ||
| ] | ||
| "google/cloud-kms": "^0.4.3" |
There was a problem hiding this comment.
Yay for specific client libraries!
| $policy->setBindings($bindings); | ||
|
|
||
| // Set the new IAM Policy. | ||
| $kms->setIamPolicy($cryptoKeyName, $policy); |
There was a problem hiding this comment.
Should this check for success?
There was a problem hiding this comment.
I believe the API will throw an error if this is not successful
| @@ -0,0 +1,64 @@ | |||
| <?php | |||
| /** | |||
| * Copyright 2018 Google Inc. | |||
There was a problem hiding this comment.
Slowly shifting to Google LLC, unless it's pending on some test automation updates?
| foreach ($policy->getBindings() as $binding) { | ||
| if ($binding->getRole() == $role) { | ||
| $members = $binding->getMembers(); | ||
| foreach ($members as $i => $existingMember) { |
There was a problem hiding this comment.
Could shrink this a couple lines with array_search(). It's looking like one of the more complicated operations. Maybe the client library should have some helper functions on a couple of these operations that require fairly boilerplate modifications?
In practice, I would wrap this in a helper library rather than inline the code anywhere.
There was a problem hiding this comment.
array_search doesn't work because $members is a protobuf object :-/
I agree about the helper functions, and I thought we might have something written in the client libraries but I couldn't find anything. Adding @dwsupplee for advice here.
3 participants
No description provided.