feat(storage): Add cname support for V4 signature #72
Conversation
googlebot
added
the
cla: yes
google/cloud/storage/blob.py
Outdated
| @@ -362,6 +362,7 @@ def generate_signed_url( | |||
| service_account_email=None, | |||
| access_token=None, | |||
| virtual_hosted_style=False, | |||
| use_cname=None, | |||
There was a problem hiding this comment.
Please use parameter name: bucket_bound_host_name. I prefer that this parameter be passed in a string and is an alias for api_access_endpoint=. I'd prefer not required two parameters to set bucket_bound_host_name as enabled.
There was a problem hiding this comment.
Thanks for your patience @HemangChothani, I have a few nits.
google/cloud/storage/blob.py
Outdated
| See: https://cloud.google.com/storage/docs/request-endpoints#cname | ||
|
|
||
| :type host_name_scheme: str | ||
| :param host_name_scheme: |
google/cloud/storage/bucket.py
Outdated
| @@ -2373,6 +2375,8 @@ def generate_signed_url( | |||
| amount of time, you can use this method to generate a URL that | |||
| is only valid within a certain time period. | |||
|
|
|||
| If ``cname`` is set as an argument of :attr:`api_access_endpoint`, ``https`` works only if using a ``CDN``. | |||
There was a problem hiding this comment.
cname no longer used in method signature should be updated.
google/cloud/storage/bucket.py
Outdated
| if ":" in bucket_bound_host_name: | ||
| api_access_endpoint = bucket_bound_host_name | ||
| else: | ||
| api_access_endpoint = "{scheme}://{cname}".format( |
There was a problem hiding this comment.
use bucket_bound_hostname to not go back and forth between cname
| @@ -2373,6 +2375,8 @@ def generate_signed_url( | |||
| amount of time, you can use this method to generate a URL that | |||
| is only valid within a certain time period. | |||
|
|
|||
| If ``cname`` is set as an argument of :attr:`api_access_endpoint`, ``https`` works only if using a ``CDN``. | |||
|
|
|||
There was a problem hiding this comment.
Add an inline example of using bucket_bound_hostname and scheme
google/cloud/storage/bucket.py
Outdated
| @@ -2355,6 +2355,8 @@ def generate_signed_url( | |||
| credentials=None, | |||
| version=None, | |||
| virtual_hosted_style=False, | |||
| bucket_bound_host_name=None, | |||
tests/unit/test__signing.py
Outdated
| @@ -744,14 +746,31 @@ def test_conformance_client(test_data): | |||
|
|
|||
| @pytest.mark.parametrize("test_data", _BUCKET_TESTS) | |||
| def test_conformance_bucket(test_data): | |||
| resource = "/{}".format(test_data["bucket"]) | |||
| _run_conformance_test(resource, test_data) | |||
| if "urlStyle" in test_data and test_data["urlStyle"] == "BUCKET_BOUND_DOMAIN": | |||
There was a problem hiding this comment.
Please update to latest release of conformance tests this value was updated to BUCKET_BOUND_HOSTNAME. Last nit.
|
Friendly ping, @HemangChothani |
|
@frankyn Do we need to add conformance tests for |
|
Good, catch, that was not implemented in #58 could you pick that up? |
|
@frankyn Yes sure, thanks. |
* feat(storage): add cname support for v4 signature * docs(storage): comment changes * feat(storage): address comment * feat(storage): doc fix * feat(storage): nit addressed * feat(storage): add conformance tests * feat(storage): nit Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
* feat(storage): add cname support for v4 signature * docs(storage): comment changes * feat(storage): address comment * feat(storage): doc fix * feat(storage): nit addressed * feat(storage): add conformance tests * feat(storage): nit Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
Fixes #11