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

test(storage): add utils for new gcs emulator #5057

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Conversation

vnghia
Copy link
Contributor

@vnghia vnghia commented Sep 15, 2020

This PR adds some utils for the new emulator ( I prefer emulator over testbench ):

  • acl.py: utils related to access control list
  • error.py: a common interface to return error code for both REST and gRPC
  • common.py: common utils used by both gRPC and REST.

The generated protos will be installed via pip from googleapis/python-storage#270

This PR is a part of #4751


This change is Reviewable

@vnghia vnghia requested a review from a team as a code owner September 15, 2020 06:53
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 15, 2020
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Sep 15, 2020
Copy link
Member

@coryan coryan left a comment

Choose a reason for hiding this comment

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

A few nits below. Do you think we should be writing unit tests for this new code? It is complex enough that maybe we should.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @vnvo2409)


google/cloud/storage/emulator/requirements.txt, line 5 at r1 (raw file):

flask==1.1.2
grpc-google-iam-v1==0.12.3
pysimdjson==3.0.0

missing newlines at the end of the file?


google/cloud/storage/emulator/utils/init.py, line 1 at r1 (raw file):

# Copyright 2020 Google Inc.

s/Google Inc./Google LLC/ here and below.


google/cloud/storage/emulator/utils/acl.py, line 1 at r1 (raw file):

# Copyright 2020 Google Inc.

s/Google Inc./Google LLC/


google/cloud/storage/emulator/utils/acl.py, line 14 at r1 (raw file):

# See the License for the specific language governing permissions and
# limitations under the License.
"""Utils related to access control"""

nit: blank line after copyright boiler plate?


google/cloud/storage/emulator/utils/acl.py, line 22 at r1 (raw file):

from google.cloud.storage_v1.proto import storage_resources_pb2 as resources_pb2

PROJECT_NUMBER = os.getenv("GCS_EMULATOR_PROJECT_NUMBER", "123456789")

We are trying to prefix all environment variables with GOOGLE_CLOUD_CPP_<SERVICE>, please use GOOGLE_CLOUD_CPP_STORAGE_EMULATOR_PROJECT_NUMBER


google/cloud/storage/emulator/utils/acl.py, line 24 at r1 (raw file):

PROJECT_NUMBER = os.getenv("GCS_EMULATOR_PROJECT_NUMBER", "123456789")
OBJECT_OWNER_ENTITY = os.getenv(
    "GCS_EMULATOR_OBJECT_OWNER_ENTITY", "user-object.owners@gmail.com"

can we use user-object.owners@example.com to avoid emailing some real person? same comments about environment variables.


google/cloud/storage/emulator/utils/acl.py, line 27 at r1 (raw file):

)
OBJECT_READER_ENTITY = os.getenv(
    "GCS_EMULATOR_OBJECT_READER_ENTITY", "user-object.viewers@gmail.com"

Ditto.


google/cloud/storage/emulator/utils/common.py, line 1 at r1 (raw file):

# Copyright 2020 Google Inc.

s/Google Inc./Google LLC/


google/cloud/storage/emulator/utils/common.py, line 14 at r1 (raw file):

# See the License for the specific language governing permissions and
# limitations under the License.
"""Common utils"""

nit: blank line.


google/cloud/storage/emulator/utils/error.py, line 1 at r1 (raw file):

# Copyright 2020 Google Inc.

s/Google Inc./Google LLC/


google/cloud/storage/emulator/utils/error.py, line 14 at r1 (raw file):

# See the License for the specific language governing permissions and
# limitations under the License.
"""Utils to raise error code and abort the server"""

nit: blank line?

@coryan coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 15, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 15, 2020
@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #5057 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5057   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files         983      983           
  Lines       76452    76453    +1     
=======================================
+ Hits        71888    71894    +6     
+ Misses       4564     4559    -5     
Impacted Files Coverage Δ
...d/pubsub/internal/subscription_lease_management.cc 98.00% <0.00%> (+0.02%) ⬆️
google/cloud/storage/parallel_upload.cc 99.61% <0.00%> (+0.38%) ⬆️
...le/cloud/storage/internal/curl_download_request.cc 79.59% <0.00%> (+0.51%) ⬆️
...ud/spanner/integration_tests/client_stress_test.cc 77.87% <0.00%> (+0.88%) ⬆️
google/cloud/internal/parse_rfc3339.cc 97.82% <0.00%> (+1.08%) ⬆️
...ogle/cloud/storage/internal/hash_validator_impl.cc 98.11% <0.00%> (+1.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18390f...ed029d7. Read the comment docs.

@vnghia
Copy link
Contributor Author

vnghia commented Sep 15, 2020

I will add tests in the next PR

@coryan coryan added the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 15, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Sep 15, 2020
@coryan
Copy link
Member

coryan commented Sep 15, 2020


google/cloud/storage/emulator/utils/init.py, line 1 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

s/Google Inc./Google LLC/ here and below.

nit: next time you go through these files, no . after LLC.

@vnghia
Copy link
Contributor Author

vnghia commented Sep 15, 2020

I copied the style of google/cloud/storage/testbench/ but it seems that that style is outdated. You think I should fix these lines in this PR or later PR ?

@coryan
Copy link
Member

coryan commented Sep 15, 2020

Later is fine.

Copy link
Member

@coryan coryan 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. Yes, the copyright notice format changed, and we are not supposed to change the old ones 🤷‍♂️

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@coryan coryan merged commit d9e7a62 into googleapis:master Sep 16, 2020
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

3 participants