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

Update interops test proto to match grpc-java. #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

creamsoup
Copy link

The test.proto is adapted from grpc-java, and stubby4 server for cloud to cloud
interops test gets same update.

  • Using protobuf provided BoolValue instead of custom one.
    the issue in the command is closed, and interops test passed
  • This change is backward compatible.

Copy link
Contributor

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

be careful here, have you checked the other languages in grpc/grpc and grpc/grpc-go can make this change?

@ejona86
Copy link
Member

ejona86 commented Oct 26, 2018

I'd feel much better if we delayed making the wrappers change until after the various languages were all pulling from the same proto. We've had trouble with the well-known protos in a few languages, since our build infrastructure is... weird... in places.

@ejona86
Copy link
Member

ejona86 commented Oct 26, 2018

My statement applies to Empty as well.

@creamsoup
Copy link
Author

changed back to use custom message types. I tested the previous version in go / c++ / python / ruby, etc. the well-known protos seem to work fine. but i agree that it still seems wiser to sync between languages first.

@creamsoup
Copy link
Author

for the BoolValue, grpc-java actually uses well known protos. Since we want to keep the custom one due to the past issue, changing the java one to use custom bool value.

@ejona86
Copy link
Member

ejona86 commented Oct 29, 2018

grpc-java using BoolValue for the moment sounds fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants