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

Add bigtable generated files #1152

Merged
merged 4 commits into from
Sep 28, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Sep 24, 2015

@tseaver These are generated with

https://github.com/dhermes/gcloud-python-bigtable/blob/7c88583a4f772701228c2c94c32a847670fe6a52/Makefile

I had to modify

https://github.com/dhermes/gcloud-python-bigtable/blob/7c88583a4f772701228c2c94c32a847670fe6a52/scripts/rewrite_imports.py

so that it rewrote imports as gcloud.bigtable instead of gcloud_bigtable.


Should I try to port the Makefile over as well? The reason I hesitate is because it requires having a valid gRPC install.

Many auxiliary files (i.e. not part of the Bigtable service
definitions) are required.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 24, 2015
@tseaver
Copy link
Contributor

tseaver commented Sep 24, 2015

I don't think there is much to review here: should we maybe be reviewing the generator first?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 24, 2015

Sure. That is contingent on whether we should add it to the repo. A Makefile is certainly not platform independent, so I hesitate a little bit. (NOTE: I want to do a similar thing with #594 but the gRPC plugin for protoc makes some imports that we can't use. I need to be less dumb using namespace packages though and maybe we could avoid import rewrites.)

@tseaver
Copy link
Contributor

tseaver commented Sep 24, 2015

Should we be checking in the .proto files?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 24, 2015

Yeah good call!

@tseaver
Copy link
Contributor

tseaver commented Sep 24, 2015

I think checking in the Makefile too is worthwhile: even if it is Unix/Linux/Ubuntu-dependent, it "documents" how the generated files are created, allowing users to figure out how to replicate on their own systems.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 24, 2015

I just added the .proto files. I'll bring in the Makefile too and we can discuss once it's in.

Do you want me to re-arrange the commits to make it more clear what's going in?

@tseaver
Copy link
Contributor

tseaver commented Sep 24, 2015

Commit order doesn't really matter to me -- I'm going to look primarily at the Makefile and one each of the .proto and _pb2.py files. If you think it is cleaner to rebase down to a single commit, that would be fine.

@tseaver
Copy link
Contributor

tseaver commented Sep 24, 2015

When the Makefile comes in, will I be able to regenerate from it (assuming protoc is on the path)?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 24, 2015

Not unless you also have the grpc_python_plugin installed. And you may need the latest and greatest version of protoc to support proto3 (was proto2 for awhile). I'm not sure about that one though.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 24, 2015

@tseaver PTAL (I added the Makefile)

@tseaver
Copy link
Contributor

tseaver commented Sep 28, 2015

LGTM -- I wish we had directions (in CONTRIBUTING.rst, maybe) about the environment needed to be able to recreate the generated files.

dhermes added a commit that referenced this pull request Sep 28, 2015
@dhermes dhermes merged commit 7c151af into googleapis:master Sep 28, 2015
@dhermes dhermes deleted the add-bigtable-generated-files branch September 28, 2015 18:48
@dhermes
Copy link
Contributor Author

dhermes commented Sep 28, 2015

Take a look through

https://github.com/dhermes/gcloud-python-bigtable/blob/master/README.md
https://github.com/dhermes/gcloud-python-bigtable/blob/master/CONTRIBUTING.md

and let me know what you'd like to be brought over.

Unfortunately (or maybe it's fortunate?) the gRPC install story is ever-evolving so the docs will be out-of-date pretty soon.

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