Skip to content

Conversation

@stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Jul 14, 2017

⚠️ Includes a breaking change for @google-cloud/common-grpc

This removes the usage of "google-proto-files" from @google-cloud/common-grpc, as well as our APIs. It is still in use by the generated code.

To Dos

  • Unit tests

Status

✔️ Handwritten APIs that load files from "protos" directory:

  • Bigtable
  • Datastore
  • Pub/Sub
  • Spanner

❌ Autogen APIs that load files from "google-proto-files" dependency:

  • Datastore
  • Pub/Sub
  • Spanner
  • Speech

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 14, 2017
@stephenplusplus stephenplusplus added api: bigtable Issues related to the Bigtable API. api: datastore Issues related to the Datastore API. api: pubsub Issues related to the Pub/Sub API. api: spanner Issues related to the Spanner API. api: speech Issues related to the Speech-to-Text API. core status: do not merge labels Jul 14, 2017
@lukesneeringer
Copy link
Contributor

Where do the protos live now?

@stephenplusplus
Copy link
Contributor Author

Datastore as an example:

packages/
  datastore/
    protos/
      google/
        api/
          **/*.proto
        datastore/
          v1/
            datastore.proto
        protobuf/
          **/*.proto
        type/
          **/*.proto

@callmehiphop
Copy link
Contributor

What does the process for updating proto files look like now? Are we just essentially copy/pasting?

@stephenplusplus
Copy link
Contributor Author

Yes, I believe that's the plan. @lukesneeringer?

@stephenplusplus stephenplusplus force-pushed the spp--remove-google-proto-files branch 2 times, most recently from 18ea4d1 to 3d6dd88 Compare July 17, 2017 18:23
@lukesneeringer
Copy link
Contributor

Yep, straight-out copy/paste is the correct thing to do.

@stephenplusplus
Copy link
Contributor Author

@callmehiphop ready for a review! Note that for testing locally, you'll have to run npm run link-common, as the updates to the modules depend on the updates to @google-cloud/common-grpc

@stephenplusplus
Copy link
Contributor Author

@lukesneeringer RE the "❌ Autogen APIs that load files from "google-proto-files" dependency" section from the first post, who can take care of this? Those generated _client.js files will need to load the protos from ../../protos/google/... (the amount of ../../s was a guess).

@callmehiphop
Copy link
Contributor

@stephenplusplus I'm getting a lot of failures locally for PubSub unit tests

TypeError: base64 is not a function
      at Function.from (native)
      at Function.from (native)
      at Subscription.formatMessage_ (src/subscription.js:364:31)
      at Function.Subscription.formatMessage_ (test/subscription.js:100:55)
      at src/subscription.js:723:29
      at Array.map (native)
      at src/subscription.js:722:8
      at Object.subscription.parent.request (test/subscription.js:658:9)
      at Subscription.pull (src/subscription.js:704:37)
      at Context.<anonymous> (test/subscription.js:869:22)

Not sure if I missed a step or something

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API. api: datastore Issues related to the Datastore API. api: pubsub Issues related to the Pub/Sub API. api: spanner Issues related to the Spanner API. api: speech Issues related to the Speech-to-Text API. cla: yes This human has signed the Contributor License Agreement. core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants