feat: Allow for topics to updated via a new method #2256
Conversation
Creates the Google\Cloud\PubSub\Topic::update method, docs and tests.
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #2256 +/- ##
========================================
Coverage ? 92.6%
Complexity ? 4467
========================================
Files ? 307
Lines ? 13323
Branches ? 0
========================================
Hits ? 12338
Misses ? 985
Partials ? 0
Continue to review full report at Codecov.
|
PubSub/src/Topic.php
Outdated
| * the allowed regions. An empty list means that no regions are | ||
| * allowed, and is not a valid configuration. * } | ||
| * @param array $options [optional] Configuration options. | ||
| * @param array $options.updateMask A list of field paths to be modified. Nested |
There was a problem hiding this comment.
Document this in the same nested manner as $topic and its sub options.
PubSub/src/Topic.php
Outdated
| * key names should be dot-separated, e.g. `messageStoragePolicy.allowedPersistenceRegions`. | ||
| * Google Cloud PHP will attempt to infer this value on your | ||
| * behalf, however modification of map fields with arbitrary keys | ||
| * (such as labels or push config attributes) requires an explicit |
There was a problem hiding this comment.
push config attributes are not applicable to this method. allowed persistence regions may be, though.
There was a problem hiding this comment.
Thanks, I changed it to reflect the correct fields.
| $topic = $topics->current(); | ||
|
|
||
| $policy = [ | ||
| 'allowedPersistenceRegions' => ['us-central1', 'us-east1'] |
There was a problem hiding this comment.
fix spacing at start of line.
| */ | ||
| public function testUpdateTopic($client) | ||
| { | ||
| $topics = $client->topics(); |
There was a problem hiding this comment.
Create a topic for the test rather than modifying an existing one. See the test immediately above for how to create a topic and enqueue it for cleanup at the end of the test run. (Same for the other system test you added)
There was a problem hiding this comment.
I don't mind using resources already setup by other tests. If we do that, though, it would be nice to use the @depends annotation and make sure the data pre-modification differs from the end result. In this case, it would probably be easier to just create a new resource.
There was a problem hiding this comment.
Updated the test to add new topics.
PubSub/src/Topic.php
Outdated
| if (!$updateMaskPaths) { | ||
| $iterator = new \RecursiveIteratorIterator(new \RecursiveArrayIterator($topic)); | ||
| foreach ($iterator as $leafValue) { | ||
| $excludes = ['name', 'kmsKeyName']; |
There was a problem hiding this comment.
Let's take kmsKeyName out of this array. If a user provides a kmsKeyName to this method, they should see the error telling them it's immutable, otherwise they may not realize that the value has not changed. It may be good to do the same for Subscription::update() w.r.t. the topic name in the near future.
There was a problem hiding this comment.
I removed this, and removed the test where I confirmed filtering out immutable fields.
Co-Authored-By: John Pedrie <[email protected]>
Co-Authored-By: John Pedrie <[email protected]>
Co-Authored-By: John Pedrie <[email protected]>
Co-Authored-By: John Pedrie <[email protected]>
| */ | ||
| public function testUpdateTopic($client) | ||
| { | ||
| $topics = $client->topics(); |
There was a problem hiding this comment.
I don't mind using resources already setup by other tests. If we do that, though, it would be nice to use the @depends annotation and make sure the data pre-modification differs from the end result. In this case, it would probably be easier to just create a new resource.
Co-Authored-By: David Supplee <[email protected]>
Co-Authored-By: John Pedrie <[email protected]>
Creates the Google\Cloud\PubSub\Topic::update method, docs and tests.