-
Notifications
You must be signed in to change notification settings - Fork 370
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
cleanup(storage): remove unused code in emulator #7048
cleanup(storage): remove unused code in emulator #7048
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7048 +/- ##
=======================================
Coverage 94.48% 94.48%
=======================================
Files 1304 1304
Lines 112319 112319
=======================================
Hits 106126 106126
Misses 6193 6193
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @coryan)
google/cloud/storage/emulator/gcs/bucket.py, line 466 at r1 (raw file):
"kind": "storage#notification", "id": "notification-%d" % random.getrandbits(16), "payload_format": "JSON_API_V1",
I'm sure you know, but I do not know why "JSON_API_V1"
is the default value
google/cloud/storage/emulator/tests/test_bucket.py, line 302 at r1 (raw file):
bucket, _ = gcs.bucket.Bucket.init(request, None) for topic in ["test-topic-1", "test-topic-2"]:
why do we need the loop?
google/cloud/storage/emulator/tests/test_bucket.py, line 311 at r1 (raw file):
get = bucket.get_notification(notification["id"], None) self.assertEqual(notification, get)
get
is a strange variable name
Since we are no longer sending gRPC requests for Bucket metadata operations, we do not need to emulate them either. This is needed because the v2/ protos do not expose these RPCs (yet).
4ae8f53
to
f8a45ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @dbolduc)
google/cloud/storage/emulator/gcs/bucket.py, line 466 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
I'm sure you know, but I do not know why `"JSON_API_V1" is the default value
It should not have been, thanks for pointing it out.
google/cloud/storage/emulator/tests/test_bucket.py, line 302 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
why do we need the loop?
Because I was going to test listing, and then I forgot. Fixed.
google/cloud/storage/emulator/tests/test_bucket.py, line 311 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
get
is a strange variable name
Fixed.
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @dbolduc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @coryan and @dbolduc)
google/cloud/storage/emulator/tests/test_bucket.py, line 319 at r2 (raw file):
for id in [n["id"] for n in expected]: bucket.delete_notification(id, None)
I think this would be easier on the eyes:
for n in expected:
bucket.delete_notification(n["id"], None)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @dbolduc)
google/cloud/storage/emulator/tests/test_bucket.py, line 319 at r2 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
for id in [n["id"] for n in expected]: bucket.delete_notification(id, None)
I think this would be easier on the eyes:
for n in expected: bucket.delete_notifications(n["id"], None)
Done.
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Since we are no longer sending gRPC requests for Bucket metadata
operations, we do not need to emulate them either. This is needed
because the v2/ protos do not expose these RPCs (yet).
Part of the work for #6982
This change is