Repository: googleapis/python-storage

Issue:
Move bucket_bound_hostname code into helper
For now we have three places in which we're using `bucket_bound_hostname`. The code is almost the same in all three places, so it'll be good to union it in single helper:
https://github.com/googleapis/python-storage/blob/616f1079e2c8b5a4c900f663a9688da64e2bc1b0/google/cloud/storage/blob.py#L517-L522

https://github.com/googleapis/python-storage/blob/616f1079e2c8b5a4c900f663a9688da64e2bc1b0/google/cloud/storage/bucket.py#L2531-L2536

https://github.com/googleapis/python-storage/blob/616f1079e2c8b5a4c900f663a9688da64e2bc1b0/google/cloud/storage/client.py#L1014-L1020

Discussion:
gcf-merge-on-green[bot]: Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR
gcf-merge-on-green[bot]: Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR
gcf-merge-on-green[bot]: Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

Patch:
diff --git a/google/cloud/storage/_helpers.py b/google/cloud/storage/_helpers.py
index 79298c5..a1075ea 100644
--- a/google/cloud/storage/_helpers.py
+++ b/google/cloud/storage/_helpers.py
@@ -22,6 +22,7 @@ from hashlib import md5
 from datetime import datetime
 import os
 
+from six.moves.urllib.parse import urlsplit
 from google.cloud.storage.constants import _DEFAULT_TIMEOUT
 
 
@@ -491,3 +492,23 @@ def _raise_if_more_than_one_set(**kwargs):
         )
 
         raise ValueError(msg)
+
+
+def _bucket_bound_hostname_url(host, scheme=None):
+    """Helper to build bucket bound hostname URL.
+
+    :type host: str
+    :param host: Host name.
+
+    :type scheme: str
+    :param scheme: (Optional) Web scheme. If passed, use it
+                   as a scheme in the result URL.
+
+    :rtype: str
+    :returns: A bucket bound hostname URL.
+    """
+    url_parts = urlsplit(host)
+    if url_parts.scheme and url_parts.netloc:
+        return host
+
+    return "{scheme}://{host}/".format(scheme=scheme, host=host)
diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py
index ab50a8f..9d91bc5 100644
--- a/google/cloud/storage/blob.py
+++ b/google/cloud/storage/blob.py
@@ -57,6 +57,7 @@ from google.cloud.exceptions import NotFound
 from google.cloud.storage._helpers import _add_generation_match_parameters
 from google.cloud.storage._helpers import _PropertyMixin
 from google.cloud.storage._helpers import _scalar_property
+from google.cloud.storage._helpers import _bucket_bound_hostname_url
 from google.cloud.storage._helpers import _convert_to_timestamp
 from google.cloud.storage._helpers import _raise_if_more_than_one_set
 from google.cloud.storage._signing import generate_signed_url_v2
@@ -516,12 +517,9 @@ class Blob(_PropertyMixin):
                 bucket_name=self.bucket.name
             )
         elif bucket_bound_hostname:
-            if ":" in bucket_bound_hostname:
-                api_access_endpoint = bucket_bound_hostname
-            else:
-                api_access_endpoint = "{scheme}://{bucket_bound_hostname}".format(
-                    scheme=scheme, bucket_bound_hostname=bucket_bound_hostname
-                )
+            api_access_endpoint = _bucket_bound_hostname_url(
+                bucket_bound_hostname, scheme
+            )
         else:
             resource = "/{bucket_name}/{quoted_name}".format(
                 bucket_name=self.bucket.name, quoted_name=quoted_name
diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py
index d94190c..78d8c5f 100644
--- a/google/cloud/storage/bucket.py
+++ b/google/cloud/storage/bucket.py
@@ -38,6 +38,7 @@ from google.cloud.storage._helpers import _scalar_property
 from google.cloud.storage._helpers import _validate_name
 from google.cloud.storage._signing import generate_signed_url_v2
 from google.cloud.storage._signing import generate_signed_url_v4
+from google.cloud.storage._helpers import _bucket_bound_hostname_url
 from google.cloud.storage.acl import BucketACL
 from google.cloud.storage.acl import DefaultObjectACL
 from google.cloud.storage.blob import Blob
@@ -2861,12 +2862,9 @@ class Bucket(_PropertyMixin):
                 bucket_name=self.name
             )
         elif bucket_bound_hostname:
-            if ":" in bucket_bound_hostname:
-                api_access_endpoint = bucket_bound_hostname
-            else:
-                api_access_endpoint = "{scheme}://{bucket_bound_hostname}".format(
-                    scheme=scheme, bucket_bound_hostname=bucket_bound_hostname
-                )
+            api_access_endpoint = _bucket_bound_hostname_url(
+                bucket_bound_hostname, scheme
+            )
         else:
             resource = "/{bucket_name}".format(bucket_name=self.name)
 
diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py
index ce178ee..4927aec 100644
--- a/google/cloud/storage/client.py
+++ b/google/cloud/storage/client.py
@@ -30,6 +30,7 @@ from google.cloud._helpers import _LocalStack, _NOW
 from google.cloud.client import ClientWithProject
 from google.cloud.exceptions import NotFound
 from google.cloud.storage._helpers import _get_storage_host
+from google.cloud.storage._helpers import _bucket_bound_hostname_url
 from google.cloud.storage._http import Connection
 from google.cloud.storage._signing import (
     get_expiration_seconds_v4,
@@ -1079,15 +1080,8 @@ class Client(ClientWithProject):
         # designate URL
         if virtual_hosted_style:
             url = "https://{}.storage.googleapis.com/".format(bucket_name)
-
         elif bucket_bound_hostname:
-            if ":" in bucket_bound_hostname:  # URL includes scheme
-                url = bucket_bound_hostname
-
-            else:  # scheme is given separately
-                url = "{scheme}://{host}/".format(
-                    scheme=scheme, host=bucket_bound_hostname
-                )
+            url = _bucket_bound_hostname_url(bucket_bound_hostname, scheme)
         else:
             url = "https://storage.googleapis.com/{}/".format(bucket_name)
 


Test patch:
diff --git a/tests/unit/test__helpers.py b/tests/unit/test__helpers.py
index dee170e..e295cbe 100644
--- a/tests/unit/test__helpers.py
+++ b/tests/unit/test__helpers.py
@@ -525,6 +525,24 @@ class Test__add_generation_match_parameters(unittest.TestCase):
             )
 
 
+class Test__bucket_bound_hostname_url(unittest.TestCase):
+    def _call_fut(self, **args):
+        from google.cloud.storage._helpers import _bucket_bound_hostname_url
+
+        return _bucket_bound_hostname_url(**args)
+
+    def test_full_hostname(self):
+        HOST = "scheme://domain.tcl/"
+        self.assertEqual(self._call_fut(host=HOST), HOST)
+
+    def test_hostname_and_scheme(self):
+        HOST = "domain.tcl"
+        SCHEME = "scheme"
+        EXPECTED_URL = SCHEME + "://" + HOST + "/"
+
+        self.assertEqual(self._call_fut(host=HOST, scheme=SCHEME), EXPECTED_URL)
+
+
 class _Connection(object):
     def __init__(self, *responses):
         self._responses = responses
diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py
index 0a5bfd9..cd0eae0 100644
--- a/tests/unit/test_blob.py
+++ b/tests/unit/test_blob.py
@@ -405,6 +405,7 @@ class Test_Blob(unittest.TestCase):
     ):
         from six.moves.urllib import parse
         from google.cloud._helpers import UTC
+        from google.cloud.storage._helpers import _bucket_bound_hostname_url
         from google.cloud.storage.blob import _API_ACCESS_ENDPOINT
         from google.cloud.storage.blob import _get_encryption_headers
 
@@ -464,12 +465,9 @@ class Test_Blob(unittest.TestCase):
                 bucket.name
             )
         elif bucket_bound_hostname:
-            if ":" in bucket_bound_hostname:
-                expected_api_access_endpoint = bucket_bound_hostname
-            else:
-                expected_api_access_endpoint = "{scheme}://{bucket_bound_hostname}".format(
-                    scheme=scheme, bucket_bound_hostname=bucket_bound_hostname
-                )
+            expected_api_access_endpoint = _bucket_bound_hostname_url(
+                bucket_bound_hostname, scheme
+            )
         else:
             expected_api_access_endpoint = api_access_endpoint
             expected_resource = "/{}/{}".format(bucket.name, quoted_name)
diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py
index 5069d87..2932036 100644
--- a/tests/unit/test_bucket.py
+++ b/tests/unit/test_bucket.py
@@ -2990,6 +2990,7 @@ class Test_Bucket(unittest.TestCase):
     ):
         from six.moves.urllib import parse
         from google.cloud._helpers import UTC
+        from google.cloud.storage._helpers import _bucket_bound_hostname_url
         from google.cloud.storage.blob import _API_ACCESS_ENDPOINT
 
         api_access_endpoint = api_access_endpoint or _API_ACCESS_ENDPOINT
@@ -3037,12 +3038,9 @@ class Test_Bucket(unittest.TestCase):
                 bucket_name
             )
         elif bucket_bound_hostname:
-            if ":" in bucket_bound_hostname:
-                expected_api_access_endpoint = bucket_bound_hostname
-            else:
-                expected_api_access_endpoint = "{scheme}://{bucket_bound_hostname}".format(
-                    scheme=scheme, bucket_bound_hostname=bucket_bound_hostname
-                )
+            expected_api_access_endpoint = _bucket_bound_hostname_url(
+                bucket_bound_hostname, scheme
+            )
         else:
             expected_api_access_endpoint = api_access_endpoint
             expected_resource = "/{}".format(parse.quote(bucket_name))