Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(storage): Add cname support for V4 signature #72

Merged
merged 14 commits into from Mar 11, 2020

Conversation

@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Feb 26, 2020

Fixes #11

@@ -362,6 +362,7 @@ def generate_signed_url(
service_account_email=None,
access_token=None,
virtual_hosted_style=False,
use_cname=None,
Copy link
Member

@frankyn frankyn Feb 26, 2020

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.

Copy link
Member

@frankyn frankyn left a comment

Thanks for your patience @HemangChothani, I have a few nits.

See: https://cloud.google.com/storage/docs/request-endpoints#cname
:type host_name_scheme: str
:param host_name_scheme:
Copy link
Member

@frankyn frankyn Mar 5, 2020

prefer scheme here.

@@ -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``.
Copy link
Member

@frankyn frankyn Mar 5, 2020

cname no longer used in method signature should be updated.

if ":" in bucket_bound_host_name:
api_access_endpoint = bucket_bound_host_name
else:
api_access_endpoint = "{scheme}://{cname}".format(
Copy link
Member

@frankyn frankyn Mar 5, 2020

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``.
Copy link
Member

@frankyn frankyn Mar 5, 2020

Add an inline example of using bucket_bound_hostname and scheme

@@ -2355,6 +2355,8 @@ def generate_signed_url(
credentials=None,
version=None,
virtual_hosted_style=False,
bucket_bound_host_name=None,
Copy link
Member

@frankyn frankyn Mar 5, 2020

use: bucket_bound_hostname

Copy link
Member

@frankyn frankyn left a comment

This is missing conformance tests for this support.

Did you want to do that in a follow-up PR or add it to this one?

Copy link
Member

@frankyn frankyn left a comment

One last nit, thanks @HemangChothani

@@ -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":
Copy link
Member

@frankyn frankyn Mar 10, 2020

Please update to latest release of conformance tests this value was updated to BUCKET_BOUND_HOSTNAME. Last nit.

@frankyn
Copy link
Member

@frankyn frankyn commented Mar 10, 2020

Friendly ping, @HemangChothani

Copy link
Member

@frankyn frankyn left a comment

LGTM

@frankyn frankyn merged commit cc853af into googleapis:master Mar 11, 2020
3 checks passed
@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Mar 11, 2020

@frankyn Do we need to add conformance tests for virtual_hosted_style?

@frankyn
Copy link
Member

@frankyn frankyn commented Mar 11, 2020

Good, catch, that was not implemented in #58 could you pick that up?

@HemangChothani
Copy link
Contributor Author

@HemangChothani HemangChothani commented Mar 11, 2020

@frankyn Yes sure, thanks.

@frankyn
Copy link
Member

@frankyn frankyn commented Mar 11, 2020

@tseaver was working on this in #57

cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* 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>
cojenco added a commit to cojenco/python-storage that referenced this issue Oct 13, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants