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.

Sign up for GitHub

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

Jump to bottom

Implement predefined acl #4757

Merged
merged 5 commits into from
Feb 27, 2018
Merged

Implement predefined acl #4757

merged 5 commits into from
Feb 27, 2018

Conversation

chemelnucfin
Copy link
Contributor

Closes #1660

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 17, 2018
@chemelnucfin chemelnucfin self-assigned this Jan 17, 2018
@chemelnucfin chemelnucfin added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: storage Issues related to the Cloud Storage API. labels Jan 17, 2018
@chemelnucfin chemelnucfin force-pushed the storage_predefined_acl branch 2 times, most recently from a3c3774 to 54bb0fc Compare January 17, 2018 22:30
tseaver
tseaver suggested changes Jan 18, 2018
View reviewed changes
storage/google/cloud/storage/blob.py Outdated
@@ -688,6 +691,9 @@ def _do_multipart_upload(self, client, stream, content_type,

if self.user_project is not None:
name_value_pairs.append(('userProject', self.user_project))
if predefined_acl is not None:
self._acl.save_predefined(predefined_acl)
name_value_pairs.append(('predefinedAcl', predefined_acl))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin force-pushed the storage_predefined_acl branch from 54bb0fc to 295eb03 Compare January 26, 2018 05:29
@chemelnucfin
Copy link
Contributor Author

@tseaver PTAL, thanks. I set the acl after the blob was created as you had suggested.

@tseaver
Copy link
Contributor

tseaver commented Jan 26, 2018

@chemelnucfin Moving the save_predefined call after the actual upload still makes two API calls. The change I asked for was to avoid using save_predefined at all, and instead to get the appropriate query parameter added to the actual upload API request. This change will require factoring out the XML->JSON parameter name code (currently inside save_predefined), and tweaking _initiate_resumable_upload and _do_multipart_upload to inject the query parameter appropriately (like for userProject).

@chemelnucfin
Copy link
Contributor Author

Sorry, I must have misunderstood you. I will take a look in a bit. Thanks.

@chemelnucfin chemelnucfin force-pushed the storage_predefined_acl branch 2 times, most recently from b970be1 to 1d0aae7 Compare January 30, 2018 23:27
@chemelnucfin
Copy link
Contributor Author

@tseaver PTAL at your convenience. Thanks.

tseaver
tseaver suggested changes Feb 1, 2018
View reviewed changes
storage/tests/unit/test_blob.py Outdated
@@ -1009,9 +1013,11 @@ def _initiate_resumable_helper(
data = b'hello hallo halo hi-low'
stream = io.BytesIO(data)
content_type = u'text/plain'
predefined_acl = 'private'

This comment was marked as spam.

Sign in to view

storage/tests/unit/test_blob.py Outdated
@@ -1180,8 +1187,9 @@ def _do_resumable_helper(self, use_size=False, num_retries=None):
client = mock.Mock(_http=transport, spec=['_http'])
stream = io.BytesIO(data)
content_type = u'text/html'
predefined_acl = 'private'

This comment was marked as spam.

Sign in to view

storage/tests/unit/test_blob.py Outdated
@@ -1225,20 +1233,21 @@ def _do_upload_helper(self, chunk_size=None, num_retries=None):
stream = mock.sentinel.stream
content_type = u'video/mp4'
size = 12345654321

predefined_acl = 'private'

This comment was marked as spam.

Sign in to view

storage/tests/unit/test_blob.py Outdated

blob = self._make_one('blob-name', bucket=None)

blob._acl = mock.create_autospec(ACL)

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin force-pushed the storage_predefined_acl branch from fb2b43b to f411292 Compare February 4, 2018 10:19
tseaver
tseaver suggested changes Feb 5, 2018
View reviewed changes
storage/tests/unit/test_blob.py Outdated
@@ -1187,7 +1190,6 @@ def _do_resumable_helper(self, use_size=False, num_retries=None):
client = mock.Mock(_http=transport, spec=['_http'])
stream = io.BytesIO(data)
content_type = u'text/html'
predefined_acl = 'private'
response = blob._do_resumable_upload(
client, stream, content_type, size, num_retries, None)

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin force-pushed the storage_predefined_acl branch from 25578fb to 00dfef0 Compare February 7, 2018 20:15
@chemelnucfin
Copy link
Contributor Author

@tseaver Hopefully this time is good. PTAL at your convenience.

@chemelnucfin chemelnucfin changed the title Storage: Implement predefined acl Implement predefined acl Feb 20, 2018
tseaver
tseaver suggested changes Feb 22, 2018
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the other changes requested, please add explicit unit tests for ACL.valiate_predefined_acl, including cases for None, a valid XML acl, a valid JSON acl, and an invalid acl.

storage/google/cloud/storage/blob.py
@@ -941,14 +965,18 @@ def upload_from_file(self, file_obj, rewind=False, size=None,
warnings.warn(_NUM_RETRIES_MESSAGE, DeprecationWarning)

_maybe_rewind(file_obj, rewind=rewind)
predefined_acl = ACL.validate_predefined(predefined_acl)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/google/cloud/storage/blob.py Outdated
@@ -1109,7 +1145,7 @@ def create_resumable_upload_session(
# to the `ResumableUpload` constructor. The chunk size only
# matters when **sending** bytes to an upload.
upload, _ = self._initiate_resumable_upload(
client, dummy_stream, content_type, size, None,
client, dummy_stream, content_type, size, None, None,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin force-pushed the storage_predefined_acl branch from 00dfef0 to 8db085b Compare February 22, 2018 23:37
@chemelnucfin
Copy link
Contributor Author

@tseaver added the unit test for validation.

@chemelnucfin
Copy link
Contributor Author

@tseaver I added the keyword arguments, PTAL and hopefully this is good!

@chemelnucfin chemelnucfin merged commit 34cedf3 into googleapis:master Feb 27, 2018
@chemelnucfin chemelnucfin deleted the storage_predefined_acl branch February 27, 2018 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees

@chemelnucfin chemelnucfin

Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chemelnucfin @tseaver @googlebot