feat(bigtable): Managed Backups wrappers - #57
Conversation
googlebot
added
the
cla: yes
kolea2
left a comment
kolea2
left a comment
There was a problem hiding this comment.
Thanks for all of the work here! Left a few comments - also, are there any integration tests?
| self.name + "/clusters/" + cluster_id, | ||
| filter_=filter_, | ||
| order_by=order_by, | ||
| page_size=page_size, |
There was a problem hiding this comment.
what is the default here if not specified?
There was a problem hiding this comment.
According to bigtable_table_admin.pb2.py,
page_size:
Number of backups to be returned in the response. If 0 or
less, defaults to the server’s maximum allowed page size.
In the list_backups method of bigtable_table_admin_client.py, this parameter defaults to None, so here we follow the same logic. But I agree, it should probably default to 0 rather than none-type, will change.
|
@kolea2 Added the integration test. To avoid repetitive LROs, everything is placed into a single comprehensive test method inside |
feat: new `__eq__` and `__ne__` convenience methods
|
@kolea2 PTAL. Replaced some string composition code with 'canonical' methods from |
SGTM for the |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """A user-friendly container for Cloud Bigtable Backup.""" |
There was a problem hiding this comment.
not sure container is the correct word here? wrapper?. Also a Cloud Bigtable Backup or Cloud Bigtable Backups perhaps?
And it seems we mix google cloud bigtable and cloud bigtable. Maybe best to be consistent?
There was a problem hiding this comment.
Agreed about the wrapper vs container as well as Google Cloud Bigtable. Not sure about the article though, since it implies a definition of notion here, rather than "a" (i.e. particular) Backup class. I've put it in for now, just must say that it's debatable.
| backup_id = match.group("backup_id") | ||
| cluster_id = match.group("cluster_id") | ||
|
|
||
| match = re.compile( |
There was a problem hiding this comment.
There is a const regex above. Should this be done similarly?
There was a problem hiding this comment.
Makes sense. We couldn't decide whether to declare re as constants, hence the two ended up in different representations) It's fixed now.
| """ | ||
| if not self._expire_time: | ||
| raise ValueError('"expire_time" parameter must be set') | ||
| # TODO: Consider implementing a method that sets a default value of |
There was a problem hiding this comment.
Can we create an issue to track this as a feature request?
There was a problem hiding this comment.
Certainly, that's the idea. I'd just wait until the initial release is merged, but please let know if you'd rather see it sooner.
| :param backup_id: The ID of the Backup to be created. | ||
|
|
||
| :type cluster_id: str | ||
| :param cluster_id: (Optional) The ID of the Cluster. Required for |
There was a problem hiding this comment.
Seems odd we have an optional but required parameter. Can we get this by retrieving the backup? This kind of seems required? What is the use case gained by making this optional? Is this just to make it easier to create a client side object?
kolea2
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
@mf2199 lgtm - would you be able to take a look at the build error? Edit: when the tests are fixed, please mark as ready for review (from draft) as well. Thanks! |
|
@kolea2 Done. According to the log, one of the errors was due to the |
5 participants
A draft/beta implementation of Managed Backups' wrappers for Google Cloud Bigtable.