refactor(storage): move 'create_bucket' implementation from 'Bucket' to 'Client'

[IlyaFaer]

Moved Bucket.create() implementation into Client.create_bucket()
Towards: #7762

[tswast]

Thanks! This change LGTM, but I'd like @crwilcox or @frankyn to give the final sign-off.

[frankyn]

Acking, and will review tomorrow. Thank you for your patience!

[frankyn]

Overall LGTM, one missing parameter and a general comment on the design.

Thank you for your patience.

[frankyn]

I missed the technical debt with user_project in create().

https://cloud.google.com/storage/docs/json_api/v1/buckets/insert

Create supports user_project.

[frankyn]

LGTM, Thanks @IlyaFaer for your patience.

I'm not sure what's causing the coverage issue. Taking a look.

[frankyn]

Pulling out coverage report:

Creating virtual environment (virtualenv) using python3.6 in .nox/cover
pip install coverage pytest-cov
coverage report --show-missing --fail-under=100
Name Stmts Miss Branch BrPart Cover Missing
----------------------------------------------------------------------------------
google/cloud/storage/__init__.py 8 0 0 0 100%
google/cloud/storage/_helpers.py 74 0 12 0 100%
google/cloud/storage/_http.py 15 0 2 0 100%
google/cloud/storage/_signing.py 145 0 70 0 100%
google/cloud/storage/acl.py 171 0 40 0 100%
google/cloud/storage/batch.py 127 0 28 0 100%
google/cloud/storage/blob.py 441 0 134 0 100%
google/cloud/storage/bucket.py 529 0 166 0 100%
google/cloud/storage/client.py 163 6 56 4 95% 405, 413-414, 417-420, 429, 404->405, 412->413, 416->417, 428->429

After moving in the logic in Bucket.create() to Client.create_bucket(). The conditional branches in client.py are not being covered by unit tests in a similar way that bucket.py is:

google-cloud-python/storage/tests/unit/test_bucket.py

Lines 610 to 819 in e206be2

	deftest_create_w_user_project(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	USER_PROJECT="user-project-123"
	client=Client(project=PROJECT)
	client._base_connection=_Connection()
	bucket=self._make_one(client, BUCKET_NAME, user_project=USER_PROJECT)
	withself.assertRaises(ValueError):
	bucket.create()
	deftest_create_w_missing_client_project(self):
	fromgoogle.cloud.storage.clientimportClient
	BUCKET_NAME="bucket-name"
	client=Client(project=None)
	bucket=self._make_one(client, BUCKET_NAME)
	withself.assertRaises(ValueError):
	bucket.create()
	deftest_create_w_explicit_project(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	OTHER_PROJECT="other-project-123"
	DATA= {"name": BUCKET_NAME}
	connection=_make_connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client, BUCKET_NAME)
	bucket.create(project=OTHER_PROJECT)
	connection.api_request.assert_called_once_with(
	method="POST",
	path="/b",
	query_params={"project": OTHER_PROJECT},
	data=DATA,
	_target_object=bucket,
	)
	deftest_create_w_explicit_location(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	LOCATION="us-central1"
	DATA= {"location": LOCATION, "name": BUCKET_NAME}
	connection=_make_connection(
	DATA, "{'location': 'us-central1', 'name': 'bucket-name'}"
	)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client, BUCKET_NAME)
	bucket.create(location=LOCATION)
	connection.api_request.assert_called_once_with(
	method="POST",
	path="/b",
	data=DATA,
	_target_object=bucket,
	query_params={"project": "PROJECT"},
	)
	self.assertEqual(bucket.location, LOCATION)
	deftest_create_hit(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	DATA= {"name": BUCKET_NAME}
	connection=_make_connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client=client, name=BUCKET_NAME)
	bucket.create()
	connection.api_request.assert_called_once_with(
	method="POST",
	path="/b",
	query_params={"project": PROJECT},
	data=DATA,
	_target_object=bucket,
	)
	deftest_create_w_extra_properties(self):
	fromgoogle.cloud.storage.clientimportClient
	BUCKET_NAME="bucket-name"
	PROJECT="PROJECT"
	CORS= [
	{
	"maxAgeSeconds": 60,
	"methods": ["*"],
	"origin": ["https://example.com/frontend"],
	"responseHeader": ["X-Custom-Header"],
	}
	]
	LIFECYCLE_RULES= [{"action": {"type": "Delete"}, "condition": {"age": 365}}]
	LOCATION="eu"
	LABELS= {"color": "red", "flavor": "cherry"}
	STORAGE_CLASS="NEARLINE"
	DATA= {
	"name": BUCKET_NAME,
	"cors": CORS,
	"lifecycle": {"rule": LIFECYCLE_RULES},
	"location": LOCATION,
	"storageClass": STORAGE_CLASS,
	"versioning": {"enabled": True},
	"billing": {"requesterPays": True},
	"labels": LABELS,
	}
	connection=_make_connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client=client, name=BUCKET_NAME)
	bucket.cors=CORS
	bucket.lifecycle_rules=LIFECYCLE_RULES
	bucket.storage_class=STORAGE_CLASS
	bucket.versioning_enabled=True
	bucket.requester_pays=True
	bucket.labels=LABELS
	bucket.create(location=LOCATION)
	connection.api_request.assert_called_once_with(
	method="POST",
	path="/b",
	query_params={"project": PROJECT},
	data=DATA,
	_target_object=bucket,
	)
	deftest_create_w_predefined_acl_invalid(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	DATA= {"name": BUCKET_NAME}
	connection=_Connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client=client, name=BUCKET_NAME)
	withself.assertRaises(ValueError):
	bucket.create(predefined_acl="bogus")
	deftest_create_w_predefined_acl_valid(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	DATA= {"name": BUCKET_NAME}
	connection=_Connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client=client, name=BUCKET_NAME)
	bucket.create(predefined_acl="publicRead")
	kw, =connection._requested
	self.assertEqual(kw["method"], "POST")
	self.assertEqual(kw["path"], "/b")
	expected_qp= {"project": PROJECT, "predefinedAcl": "publicRead"}
	self.assertEqual(kw["query_params"], expected_qp)
	self.assertEqual(kw["data"], DATA)
	deftest_create_w_predefined_default_object_acl_invalid(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	DATA= {"name": BUCKET_NAME}
	connection=_Connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client=client, name=BUCKET_NAME)
	withself.assertRaises(ValueError):
	bucket.create(predefined_default_object_acl="bogus")
	deftest_create_w_predefined_default_object_acl_valid(self):
	fromgoogle.cloud.storage.clientimportClient
	PROJECT="PROJECT"
	BUCKET_NAME="bucket-name"
	DATA= {"name": BUCKET_NAME}
	connection=_Connection(DATA)
	client=Client(project=PROJECT)
	client._base_connection=connection
	bucket=self._make_one(client=client, name=BUCKET_NAME)
	bucket.create(predefined_default_object_acl="publicRead")
	kw, =connection._requested
	self.assertEqual(kw["method"], "POST")
	self.assertEqual(kw["path"], "/b")
	expected_qp= {"project": PROJECT, "predefinedDefaultObjectAcl": "publicRead"}
	self.assertEqual(kw["query_params"], expected_qp)
	self.assertEqual(kw["data"], DATA)

I'm wondering, given you've moved over implementation from Bucket to Client, the same should be done with unit tests. I also don't see the deletion of implementation from Bucket after looking at the coverage issue.

[IlyaFaer]

@frankyn, that's strange a little. Bucket tests are still running, and they execute code lines in Client.create_bucket() - coverage just uses data about executed lines, no matter the tests, as I know. But okay, I've copied tests to client, got 100%. bucket.create() is still working, so I'd say we should keep all of the bucket tests until method movement completion (I'm gonna add deprecations for all methods, which were moved, in next PR), WDYT?

[frankyn]

We will need to update samples to use new methods before merging/releasing the deprecation. Otherwise SGTM.

[IlyaFaer]

We will need to update samples to use new methods before merging/releasing the deprecation.

No problem, I had added comment into original issue not to forget.