chore: move firestore to use GAPICBazel and regenerate #187

crwilcox · 2020-09-16T17:24:10Z

No description provided.

synth.py

…gapic-bazel

…to gapic-bazel

crwilcox · 2020-09-16T21:18:29Z

google/cloud/firestore_admin_v1/py.typed

@@ -1,2 +1,2 @@
 # Marker file for PEP 561.
-# The google-firestore-admin package uses inline types.
+# The google-cloud-firestore-admin package uses inline types.


I am guessing this is not easily changed for multi-gen packages?

I'm going to put in feature request to allow for custom package names. Sometimes I want to change the name for other reasons (to insert a hyphen in the middle of a reallylongapiname for example). Getting this right is important for _GAPIC_LIBRARY_VERSION.

synth.py

crwilcox · 2020-09-16T21:58:18Z

New Unit test failures that are inline with what @tseaver was seeing.

=========================== short test summary info ============================
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options[FirestoreClient-FirestoreGrpcTransport-grpc]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options[FirestoreAsyncClient-FirestoreGrpcAsyncIOTransport-grpc_asyncio]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options_scopes[FirestoreClient-FirestoreGrpcTransport-grpc]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options_scopes[FirestoreAsyncClient-FirestoreGrpcAsyncIOTransport-grpc_asyncio]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options_credentials_file[FirestoreClient-FirestoreGrpcTransport-grpc]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options_credentials_file[FirestoreAsyncClient-FirestoreGrpcAsyncIOTransport-grpc_asyncio]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_client_client_options_from_dict
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_list_collection_ids
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_list_collection_ids_async
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_base_transport
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_base_transport_with_credentials_file
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_auth_adc
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_transport_auth_adc
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_grpc_transport_channel_mtls_with_client_cert_source
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_grpc_asyncio_transport_channel_mtls_with_client_cert_source
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_grpc_transport_channel_mtls_with_adc[mtls.squid.clam.whelk]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_grpc_transport_channel_mtls_with_adc[mtls.squid.clam.whelk:443]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_grpc_asyncio_transport_channel_mtls_with_adc[mtls.squid.clam.whelk]
FAILED tests/unit/gapic/firestore_v1/test_firestore_v1.py::test_firestore_grpc_asyncio_transport_channel_mtls_with_adc[mtls.squid.clam.whelk:443]
19 failed, 1100 passed, 6 warnings in 14.11s
nox > Command py.test --quiet --cov=google.cloud.firestore --cov=google.cloud --cov=tests.unit --cov-append --cov-config=.coveragerc --cov-report= --cov-fail-under=0 tests/unit failed with exit code 1
nox > Session unit-3.8 failed.

crwilcox · 2020-09-16T23:29:07Z

Coverage is down, but otherwise tests are good after merging the two test files and using the generated one as recommended by generator team

nox > Session docfx was successful.
nox > Ran multiple sessions:
nox > * lint: success
nox > * blacken: success
nox > * lint_setup_py: success
nox > * unit-3.6: success
nox > * unit-3.7: success
nox > * unit-3.8: success
nox > * system-3.7: success
nox > * cover: failed
nox > * docs: success
nox > * docfx: success
cleanup

crwilcox · 2020-09-17T17:40:35Z

I think coverage hasn't fell, so much as it was already too low. This issue is still open from @rafilong 's work. #92

busunkim96

LGTM, please also bump the dep google-api-core>=1.22.1 in setup.py.

busunkim96 · 2020-09-18T17:49:24Z

google/cloud/firestore_admin_v1/py.typed

@@ -1,2 +1,2 @@
 # Marker file for PEP 561.
-# The google-firestore-admin package uses inline types.
+# The google-cloud-firestore-admin package uses inline types.


I'm going to put in feature request to allow for custom package names. Sometimes I want to change the name for other reasons (to insert a hyphen in the middle of a reallylongapiname for example). Getting this right is important for _GAPIC_LIBRARY_VERSION.

tseaver · 2020-09-18T18:50:03Z

@crwilcox Do you want to force-merge this and then scramble to fix coverage? Or add a temporary hack to synth.py to drop the required coverage level?

crwilcox · 2020-09-18T19:03:25Z

@tseaver I forced it in. We have an issue to fix coverage that should likely be prioritized. Though I think if it blocks anything else I am okay dropping to 97% for a time

tseaver · 2020-09-18T19:00:23Z

google/cloud/firestore_admin_v1/services/firestore_admin/async_client.py

-from google.api_core import operation as ga_operation  # type: ignore
-from google.api_core import operation_async  # type: ignore
+from google.api_core import operation as ga_operation
+from google.api_core import operation_async


This is going to break pytype -- I can't tell why it isn't being run in https://source.cloud.google.com/results/invocations/02606fdf-7059-497e-931f-b5c4192081d1/targets/cloud-devrel%2Fclient-libraries%2Fpython%2Fgoogleapis%2Fpython-firestore%2Fpresubmit%2Fpresubmit/log

The fix for it is in [this PR], released in gapic-generator-python 0.33.2. Do we need to update the bazel config to use the newer version?

tseaver · 2020-09-18T19:00:51Z

google/cloud/firestore_admin_v1/services/firestore_admin/client.py

-from google.api_core import operation_async  # type: ignore
+from google.api_core import operation as ga_operation
+from google.api_core import operation
+from google.api_core import operation_async


Likewise here.

tseaver · 2020-09-18T19:06:07Z

scripts/fixup_firestore_v1_keywords.py

@@ -55,6 +55,7 @@ class firestoreCallTransformer(cst.CSTTransformer):
    'run_query': ('parent', 'structured_query', 'transaction', 'new_transaction', 'read_time', ),
    'update_document': ('document', 'update_mask', 'mask', 'current_document', ),
    'write': ('database', 'stream_id', 'writes', 'stream_token', 'labels', ),
+


Odd that black didn't correct this -- do we not run it against the scripts directory?

tseaver · 2020-09-18T19:12:34Z

setup.py

@@ -25,7 +25,7 @@
 version = "2.0.0-dev1"
 release_status = "Development Status :: 5 - Production/Stable"
 dependencies = [
-    "google-api-core[grpc] >= 1.21.0, < 2.0.0dev",
+    "google-api-core[grpc] >= 1.22.1, < 2.0.0dev",


The 1.22.1 release is odd to have as a minimum: it changes only docstrings. Seems like we would want 1.22.0 (to pick up the feature, "allow quota project to be passed to create_channel"), or else 1.22.2 (bugfix, "only add quota project id if supported").

crwilcox added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 16, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 16, 2020

crwilcox force-pushed the gapic-bazel branch from 38b0cec to e01327f Compare September 16, 2020 17:27

tseaver marked this pull request as draft September 16, 2020 18:24

fix: synth changes as local bazel targets adjusted

462dc60

crwilcox force-pushed the gapic-bazel branch from 3a5d927 to 462dc60 Compare September 16, 2020 20:29

tseaver reviewed Sep 16, 2020

View reviewed changes

synth.py Outdated Show resolved Hide resolved

tseaver reviewed Sep 16, 2020

View reviewed changes

synth.py Show resolved Hide resolved

tseaver reviewed Sep 16, 2020

View reviewed changes

synth.py Show resolved Hide resolved

crwilcox added 11 commits September 16, 2020 20:44

chore: generator output

96b35c0

Merge branch 'master' into gapic-bazel

63d120f

Merge branch 'master' of github.com:googleapis/python-firestore into …

ee0d404

…gapic-bazel

Merge branch 'gapic-bazel' of github.com:crwilcox/python-firestore in…

e6a6c34

…to gapic-bazel

chore: blacken synth.py

e28c215

fix: generate without iam methods

0a9aa19

fix: bring back replacements for admin packagename

da8dd58

fix: adjust bazel build removing synth.py edits

4ca654d

fix: undo changes to __init__.py

7ec6db1

fix: adjust admin paths

0e5ec94

fix: add scripts

a72c465

crwilcox commented Sep 16, 2020

View reviewed changes

test: for now, don't run pytype in ci

5161ae6

test: remove old gen test

3555fb3

product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Sep 17, 2020

crwilcox marked this pull request as ready for review September 17, 2020 17:42

crwilcox removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 18, 2020

busunkim96 approved these changes Sep 18, 2020

View reviewed changes

chore(deps): bump google-api-core

d87e50c

crwilcox merged commit 4861b63 into googleapis:master Sep 18, 2020

tseaver reviewed Sep 18, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move firestore to use GAPICBazel and regenerate #187

chore: move firestore to use GAPICBazel and regenerate #187

crwilcox commented Sep 16, 2020 •

edited

Loading

crwilcox Sep 16, 2020

busunkim96 Sep 18, 2020

crwilcox commented Sep 16, 2020

crwilcox commented Sep 16, 2020

crwilcox commented Sep 17, 2020

busunkim96 left a comment

busunkim96 Sep 18, 2020

tseaver commented Sep 18, 2020

crwilcox commented Sep 18, 2020

tseaver Sep 18, 2020

tseaver Sep 18, 2020

tseaver Sep 18, 2020

tseaver Sep 18, 2020

chore: move firestore to use GAPICBazel and regenerate #187

chore: move firestore to use GAPICBazel and regenerate #187

Conversation

crwilcox commented Sep 16, 2020 • edited Loading

crwilcox Sep 16, 2020

Choose a reason for hiding this comment

busunkim96 Sep 18, 2020

Choose a reason for hiding this comment

crwilcox commented Sep 16, 2020

crwilcox commented Sep 16, 2020

crwilcox commented Sep 17, 2020

busunkim96 left a comment

Choose a reason for hiding this comment

busunkim96 Sep 18, 2020

Choose a reason for hiding this comment

tseaver commented Sep 18, 2020

crwilcox commented Sep 18, 2020

tseaver Sep 18, 2020

Choose a reason for hiding this comment

tseaver Sep 18, 2020

Choose a reason for hiding this comment

tseaver Sep 18, 2020

Choose a reason for hiding this comment

tseaver Sep 18, 2020

Choose a reason for hiding this comment

crwilcox commented Sep 16, 2020 •

edited

Loading