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): move 'create_bucket' implementation from 'Bucket' to 'Client' #8604

Merged

Conversation

IlyaFaer
Copy link

@IlyaFaer IlyaFaer commented Jul 3, 2019

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 3, 2019
@IlyaFaer IlyaFaer marked this pull request as ready for review July 3, 2019 15:15
@tseaver tseaver added the api: storage Issues related to the Cloud Storage API. label Jul 3, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 4, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 4, 2019
@sduskis sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 9, 2019
@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 10, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 10, 2019
@IlyaFaer IlyaFaer changed the title Storage: client redesign proposal #7762 Storage: client redesign proposal Jul 26, 2019
@IlyaFaer IlyaFaer requested a review from tswast August 1, 2019 11:10
storage/google/cloud/storage/bucket.py Outdated Show resolved Hide resolved
storage/tests/unit/test_bucket.py Outdated Show resolved Hide resolved
@IlyaFaer IlyaFaer requested a review from tswast September 9, 2019 08:16
@tseaver tseaver changed the title Storage: client redesign proposal Storage: Move 'create_bucket' implementation from 'Bucket' to 'Client'. Sep 9, 2019
@IlyaFaer IlyaFaer changed the title Storage: Move 'create_bucket' implementation from 'Bucket' to 'Client'. refactor(storage): move 'create_bucket' implementation from 'Bucket' to 'Client' Oct 28, 2019
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

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

@frankyn
Copy link
Member

frankyn commented Oct 30, 2019

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

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

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

Thank you for your patience.

storage/google/cloud/storage/client.py Show resolved Hide resolved
storage/google/cloud/storage/client.py Show resolved Hide resolved
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

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.

storage/google/cloud/storage/client.py Outdated Show resolved Hide resolved
@IlyaFaer IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2019
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @IlyaFaer for your patience.

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

@frankyn
Copy link
Member

frankyn commented Nov 5, 2019

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:

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 IlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 5, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 5, 2019
@IlyaFaer
Copy link
Author

IlyaFaer commented Nov 5, 2019

@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?

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

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

@IlyaFaer
Copy link
Author

IlyaFaer commented Nov 8, 2019

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.

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.

8 participants