Phase 1 Update create_bucket to take Bucket object within Storage Client #7820

lbristol88 · 2019-04-30T05:33:33Z

Part of issue: #7762

lbristol88 · 2019-04-30T18:04:58Z

tswast · 2019-04-30T18:24:18Z

storage/tests/unit/test_client.py

+        from google.cloud.exceptions import Conflict
+        from google.cloud.storage.bucket import Bucket
+
+        PROJECT = "PROJECT"


nit: since these are just local variables, use lower-case letters.

Changed variable names to lower-case on all tests I'm working on.

tswast · 2019-04-30T18:26:15Z

storage/tests/unit/test_client.py

+            ]
+        )
+        data = {"error": {"message": "Conflict"}}
+        sent = {"name": BUCKET_NAME}


nit: I recommend moving this down near the assertion and calling it json_expected to align with json_sent.

Renamed sent to json_expected on all relevant tests for consistency and moved down as directed.

tswast · 2019-04-30T18:33:23Z

storage/tests/unit/test_client.py

+                "b?project=%s" % (PROJECT,),
+            ]
+        )
+        sent = {"name": BUCKET_NAME, "billing": {"requesterPays": True}}


In addition to requesterPays, let's set some properties on the bucket object in this test. The properties I most want to see are

~~location - Which region is this bucket belonging to?~~ (actually, that's not settable yet. Let's add that to the list in the redesign doc if it's not there yet)

storage_class - What kind of bucket is this?

requester_pays - Leave the argument to create_bucket unset and set it on the Bucket, instead. (We should deprecate requester_pays on create_bucket. Add that to the redesign list, too.)

Added storage_class property and adjusted requester_pays as directed. Included requester_pays and location comment in the redesign doc.

…dback.

tswast · 2019-04-30T22:01:27Z

storage/google/cloud/storage/bucket.py

@@ -398,7 +398,7 @@ class Bucket(_PropertyMixin):
    https://cloud.google.com/storage/docs/storage-classes
    """

-    def __init__(self, client, name=None, user_project=None):
+    def __init__(self, client=None, name=None, user_project=None):


Let's make this update as a separate PR, as I think we'll want to show better error messages in the existing Bucket methods when client isn't supplied (for example, say what method to call instead on Client).

Took this file out of review - please disregard, thanks!

tswast · 2019-04-30T22:03:59Z

storage/google/cloud/storage/client.py

@@ -202,7 +202,7 @@ def batch(self):
        """
        return Batch(client=self)

-    def get_bucket(self, bucket_name):
+    def get_bucket(self, bucket_or_name):


I'd prefer this to be a separate PR, too, but it's okay this once. In general a PR should correspond to 1 easily-described change (new feature / bug fix). Now this PR has 2 new features (but they are related, at least).

Took this file out of review - please disregard, thanks!

tswast · 2019-04-30T22:42:50Z

storage/google/cloud/storage/client.py

@@ -265,8 +266,8 @@ def create_bucket(self, bucket_name, requester_pays=None, project=None):
        To set additional properties when creating a bucket, such as the
        bucket location, use :meth:`~.Bucket.create`.

-        :type bucket_name: str
-        :param bucket_name: The bucket name to create.
+        :type bucket_or_name: str or resource


Could you make the docstring for create_bucket follow the same pattern as we use in BigQuery. These :type: and :param: are Sphinx/RST-style, but we are migrating to Google-style.

Docs on Google-style are at https://sphinxcontrib-napoleon.readthedocs.io/en/latest/ with an example file at https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html#example-google

We had to do this for BigQuery too. Basically, as we update each method we migrate it to the Google-style.

Updated docstring to reflect Google-style.

tswast · 2019-05-01T22:10:11Z

Python 2.7 test failure is due to #7841

____________ TestStorageNotificationCRUD.test_notification_explicit ____________
self = <tests.system.TestStorageNotificationCRUD testMethod=test_notification_explicit>
    def setUp(self):
        self.case_buckets_to_delete = []
>       self._initialize_topic()
tests/system.py:1181:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/system.py:1166: in _initialize_topic
    from google.cloud.pubsub_v1 import PublisherClient
../pubsub/google/cloud/pubsub_v1/__init__.py:19: in <module>
    from google.cloud.pubsub_v1 import subscriber
../pubsub/google/cloud/pubsub_v1/subscriber/__init__.py:17: in <module>
    from google.cloud.pubsub_v1.subscriber.client import Client
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
    from __future__ import absolute_import
    import pkg_resources
    import os
    import grpc
    from google.api_core import grpc_helpers
    from google.oauth2 import service_account
    from google.cloud.pubsub_v1 import _gapic
    from google.cloud.pubsub_v1 import types
    from google.cloud.pubsub_v1.gapic import subscriber_client
    from google.cloud.pubsub_v1.gapic.transports import subscriber_grpc_transport
    from google.cloud.pubsub_v1.subscriber import futures
>   from google.cloud.pubsub_v1.subscriber._protocol import streaming_pull_manager
E     File "/tmpfs/src/github/google-cloud-python/pubsub/google/cloud/pubsub_v1/subscriber/_protocol/streaming_pull_manager.py", line 198
E       print(f"{messages_percent}, {bytes_percent}")
E                                                  ^
E   SyntaxError: invalid syntax

tseaver · 2019-05-01T22:30:49Z

@tswast Hmm, I wasn't aware that this redesign had actually been approved: in fact I asked about that in yesterdays storage sync call, and got a "no, still under consideration."

tswast · 2019-05-01T23:45:50Z

@tseaver I got the 👍 from @frankyn , who's the DPE that owns GCS.

We don't plan on making any breaking changes in the client with the redesign. The redesign is intended to ease the eventual transition to gRPC.

frankyn · 2019-05-02T05:33:28Z

Sorry @tseaver, I did check it off with @tswast but didn't follow-up on the Github issue.

I'm also not sure about the meeting you're referring. @crwilcox is this a new spin off from the original libraries weekly?

Update create_bucket to take Bucket object

1189b2f

lbristol88 requested a review from crwilcox as a code owner April 30, 2019 05:33

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

tswast reviewed Apr 30, 2019

View reviewed changes

lbristol88 added 2 commits April 30, 2019 13:03

Updated changes based on feedback

7a38746

Updated test with changes per feedback

fb45008

crwilcox added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2019

lbristol88 added 2 commits April 30, 2019 14:57

Adjusted variable names for consistency and updated test based on fee…

e5c4860

…dback.

Made client argument be optional in Bucket constructor.

4c94e6c

tswast reviewed Apr 30, 2019

View reviewed changes

tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2019

tswast reviewed Apr 30, 2019

View reviewed changes

Removed changes for future PR

7bf2cc6

tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2019

tswast reviewed Apr 30, 2019

View reviewed changes

lbristol88 added 2 commits May 1, 2019 11:08

Updating docstring to reflect new google style

ef06ffd

Updating docstring to match Google style guide for create_bucket

5eb176b

tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2019

tswast approved these changes May 1, 2019

View reviewed changes

tswast merged commit ad2cbe0 into googleapis:master May 1, 2019

anguillanneuf mentioned this pull request May 1, 2019

Clean up bebugging code #7843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase 1 Update create_bucket to take Bucket object within Storage Client #7820

Phase 1 Update create_bucket to take Bucket object within Storage Client #7820

lbristol88 commented Apr 30, 2019 •

edited

Loading

lbristol88 commented Apr 30, 2019

tswast Apr 30, 2019

lbristol88 Apr 30, 2019

tswast Apr 30, 2019

lbristol88 Apr 30, 2019

tswast Apr 30, 2019

lbristol88 Apr 30, 2019

tswast Apr 30, 2019

lbristol88 Apr 30, 2019

tswast Apr 30, 2019

lbristol88 Apr 30, 2019

tswast Apr 30, 2019

lbristol88 May 1, 2019

tswast commented May 1, 2019

tseaver commented May 1, 2019

tswast commented May 1, 2019 •

edited

Loading

frankyn commented May 2, 2019

Phase 1 Update create_bucket to take Bucket object within Storage Client #7820

Phase 1 Update create_bucket to take Bucket object within Storage Client #7820

Conversation

lbristol88 commented Apr 30, 2019 • edited Loading

lbristol88 commented Apr 30, 2019

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

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

tswast commented May 1, 2019

tseaver commented May 1, 2019

tswast commented May 1, 2019 • edited Loading

frankyn commented May 2, 2019

lbristol88 commented Apr 30, 2019 •

edited

Loading

tswast commented May 1, 2019 •

edited

Loading