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

	def test_create_w_user_project(self):
	from google.cloud.storage.client import Client
	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)
	with self.assertRaises(ValueError):
	bucket.create()
	def test_create_w_missing_client_project(self):
	from google.cloud.storage.client import Client
	BUCKET_NAME = "bucket-name"
	client = Client(project=None)
	bucket = self._make_one(client, BUCKET_NAME)
	with self.assertRaises(ValueError):
	bucket.create()
	def test_create_w_explicit_project(self):
	from google.cloud.storage.client import Client
	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,
	)
	def test_create_w_explicit_location(self):
	from google.cloud.storage.client import Client
	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)
	def test_create_hit(self):
	from google.cloud.storage.client import Client
	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,
	)
	def test_create_w_extra_properties(self):
	from google.cloud.storage.client import Client
	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,
	)
	def test_create_w_predefined_acl_invalid(self):
	from google.cloud.storage.client import Client
	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)
	with self.assertRaises(ValueError):
	bucket.create(predefined_acl="bogus")
	def test_create_w_predefined_acl_valid(self):
	from google.cloud.storage.client import Client
	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)
	def test_create_w_predefined_default_object_acl_invalid(self):
	from google.cloud.storage.client import Client
	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)
	with self.assertRaises(ValueError):
	bucket.create(predefined_default_object_acl="bogus")
	def test_create_w_predefined_default_object_acl_valid(self):
	from google.cloud.storage.client import Client
	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.