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

refactor(storage): deprecate Bucket.create() and requester_pays #9893

Closed
wants to merge 8 commits into from

Conversation

IlyaFaer
Copy link

Towards #7762

Completing steps 1, 2 and 3 from the design doc:

image

@IlyaFaer IlyaFaer added the api: storage Issues related to the Cloud Storage API. label Nov 26, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 26, 2019
@IlyaFaer IlyaFaer changed the title refactor(storage): deprecate Bucket.create() and requester_pays. refactor(storage): deprecate Bucket.create() and requester_pays Nov 26, 2019
@@ -607,217 +607,6 @@ def api_request(cls, *args, **kwargs):
expected_cw = [((), expected_called_kwargs)]
self.assertEqual(_FakeConnection._called_with, expected_cw)

def test_create_w_user_project(self):
Copy link
Author

@IlyaFaer IlyaFaer Nov 26, 2019

Choose a reason for hiding this comment

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

Most part of these tests had already been moved into client_test.py in this PR. Some will be moved with the current one

@@ -660,7 +751,7 @@ def test_create_bucket_w_string_success(self):
http = _make_requests_session([_make_json_response(data)])
client._http_internal = http

bucket = client.create_bucket(bucket_name, requester_pays=True)
Copy link
Author

Choose a reason for hiding this comment

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

Arg is now deprecated

project=project,
location=location,
predefined_acl=predefined_acl,
predefined_default_object_acl=predefined_default_object_acl,
Copy link
Author

Choose a reason for hiding this comment

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

Deleting old implementation and mirroring to Client.create_bucket()

@IlyaFaer IlyaFaer requested review from crwilcox, frankyn and tseaver and removed request for crwilcox November 27, 2019 07:52
@IlyaFaer IlyaFaer marked this pull request as ready for review November 27, 2019 07:52
@tswast
Copy link
Contributor

tswast commented Nov 27, 2019

Are there any code samples (here or in https://github.com/GoogleCloudPlatform/python-docs-samples/tree/master/storage) that still use these methods? If so, we should wait to deprecate until those are updated.

Also, we should make sure every API request you can make on Bucket can also be done from Client. It'll make for a much clearer developer story when Bucket can be treated as a dumb resource object.

@IlyaFaer
Copy link
Author

@tswast, yeah, I've checked python-docs-samples and didn't find any mentions of bucket.create() and requester_pays arg, only client.create_bucket().

Also, we should make sure every API request you can make on Bucket can also be done from Client. It'll make for a much clearer developer story when Bucket can be treated as a dumb resource object.

Well, I'm following the design doc for now, pushing piece by piece, as it's a lot of changes. If we need to mirror calls from Client() to Bucket(), maybe we should try to play around with getattr() and just make one redirecting function?

@tswast
Copy link
Contributor

tswast commented Dec 17, 2019

maybe we should try to play around with getattr() and just make one redirecting function?

In my opinion, piece-by-piece as you're currently doing will make it easiest for others to contribute / onboard new teammembers.

@IlyaFaer
Copy link
Author

Friendly ping. This PR is waiting for review

@plamut
Copy link
Contributor

plamut commented Feb 10, 2020

Please reopen in the new repo, thanks!

@IlyaFaer
Copy link
Author

Moved to googleapis/python-storage#49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants