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

chore: run template copying last in synthtool #1071

Merged
merged 5 commits into from
May 7, 2020
Merged

Conversation

thebrianchen
Copy link

No description provided.

@thebrianchen thebrianchen self-assigned this May 5, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 5, 2020
@thebrianchen thebrianchen changed the base branch from master to node10 May 5, 2020 23:14
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Question: Did you run synthtool again? I was expecting an update to the metadata file.
It probably makes sense to regenerate all files for this type of PR (and revert all bulk writer removals?).

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #1071 into node10 will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           node10    #1071      +/-   ##
==========================================
+ Coverage   98.59%   98.69%   +0.09%     
==========================================
  Files          27       27              
  Lines       17558    17677     +119     
  Branches     1375     1382       +7     
==========================================
+ Hits        17311    17446     +135     
+ Misses        244      228      -16     
  Partials        3        3              
Impacted Files Coverage Δ
dev/src/v1/firestore_admin_client.ts 98.52% <100.00%> (+0.80%) ⬆️
dev/src/write-batch.ts 100.00% <100.00%> (ø)
dev/src/bulk-writer.ts 98.79% <0.00%> (-0.02%) ⬇️
dev/src/index.ts 98.22% <0.00%> (+0.11%) ⬆️
dev/src/pool.ts 98.67% <0.00%> (+0.82%) ⬆️

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 880d5a9...af82b61. Read the comment docs.

@thebrianchen
Copy link
Author

thebrianchen commented May 6, 2020

Running synthtool on the current node10 branch results in some protos being generated. In this PR, I took the following steps:

  1. Modify update.sh to point to my remote repo with BatchWrite protos.
  2. Run synthtool on the base node10 branch.
  3. Move template copying to the end of synth.py.
  4. Re-run synthtool.
  5. Revert manual changes that we make after synthtool is run.

If this is confusing, happy to re-order commits and/or separate out into a 2 part PR.

@@ -955,6 +956,96 @@ export class FirestoreClient {
this.initialize();
return this.innerApiCalls.createDocument(request, options, callback);
}
batchWrite(
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't make this public. Thoughts on how to resolve this?

Copy link
Author

Choose a reason for hiding this comment

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

Deleted the batchWrite code in firestore_client and its corresponding usages in gapic_firestore_v1.ts.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thanks!

@thebrianchen thebrianchen merged commit 27b6fe2 into node10 May 7, 2020
@thebrianchen thebrianchen deleted the bc/synth-last branch May 7, 2020 20:58
schmidt-sebastian added a commit that referenced this pull request Jun 24, 2020
* fix!: mark v1beta1 client as deprecated (#937)

* feat!: use QueryDocumentSnapshot in FirestoreDataConverter (#965)

* deps: update to gts 2.x (#1013)

* chore!: update settings for Node 10 (#1019)

* deps: drop through2 (#1014)

* feat: support BigInt (#1016)

* fix: make update.sh work on Linux (#1043)

* fix: only use BigInt in BigInt system test (#1044)

* fix: make pbjs compile admin proto again (#1045)

* Add BulkWriter (#1055)

* docs: Add documentation for FirestoreDataConverter (#1059)

* chore: enforce return types (#1065)

* fix: add generic to Firestore.getAll() (#1066)

* chore: remove internal WriteOp (#1067)

* chore: add linter checks for it|describe.only (#1068)

* fix: handle terminate in BulkWriter (#1070)

* chore: run template copying last in synthtool (#1071)

* feat: Firestore Bundles implementation (#1078)

* feat: add support for set() with SetOptions when using FirestoreDataConverter (#1087)

* feat: Add totalDocuments and totalBytes to bundle metadata. (#1085)

* feat: Add totalDocuments and totalBytes to bundle metadata.

* fix: Better comment

* fix: Better testing.

* fix: Improve metadata testing.

* fix: incomplete expect in rate-limiter test (#1092)

* Remove BatchWrite proto, fix conformance tests

* chore: use public API types internally (#1100)

* feat: add Partition and BatchWrite protos (#1110)

* fix: remove GCF transaction fallback (#1112)

* fix: add BulkWriter integration tests, java backport changes, delete fix (#1117)

* chore: merge master (#1218)

* chore: add eslint check for console.log statements (#1229)

* fix: another attempt at fixing the flaky BulkWriter test (#1228)

* Fix comment

* Renames

* Test fix

* Fix unit tests

Co-authored-by: Brian Chen <chenbrian@google.com>
Co-authored-by: wu-hui <53845758+wu-hui@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants