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 ruby client utility for extracting Google::Rpc::Status from GRPC::Status #12452

Merged
merged 1 commit into from
Sep 19, 2017

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Sep 8, 2017

cc @geigerj @qingling128

Looking for feedback on this right now.

@grpc-kokoro
Copy link

[trickle] No significant performance differences

@qingling128
Copy link

Thanks for the heads up, @apolcyn !

@grpc-kokoro
Copy link

[microbenchmarks] No significant performance differences

@apolcyn
Copy link
Contributor Author

apolcyn commented Sep 13, 2017

PTAL

There seems to be some differences between rubocop on these tests and my dev environment which I'm not sure is the exact cause yet, but is causing some delayed rubocop errors. Assuming those are now fixed, I think this should be ready for a look. rubocop errors should be resolved now, dev branches just need to be rebased to pick up new rubocop version

IMO the most tricky part of this was the added dependency on googleapis-common-protos-types and the loading of the google-protobuf message in the new utility file. But since we already have the google-protobuf dependency in the gemspec, and because there is no runtime loading of those protobuf messages unless the new utility used, this should not break any current usage.

@geigerj
Copy link

geigerj commented Sep 18, 2017

FYI @qingling128

Any update on this PR, which is blocking the fix for googleapis/google-cloud-ruby#1438?

@qingling128
Copy link

Thanks for checking @geigerj !
Hmm, seems there are some failed tests and it has not been reviewed yet.

Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

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

LGTM. Just make sure to open an issue to (eventually) add the corresponding server-side code.

@apolcyn apolcyn force-pushed the google_rpc_status_in_ruby branch from f9d0447 to a594ba0 Compare September 18, 2017 20:59
@apolcyn
Copy link
Contributor Author

apolcyn commented Sep 18, 2017

squashing commits down before mering

@apolcyn
Copy link
Contributor Author

apolcyn commented Sep 18, 2017

mac tests saw pre-existing #12538, but sanity tests failed to build the docker image, so re-running sanity

@apolcyn
Copy link
Contributor Author

apolcyn commented Sep 19, 2017

interop tests passed but the test failed because of

ERROR: [GitHub Commit Status Setter] - {"message":"No commit found for SHA: eb464d3e6ba4b7dd04eb1058edddd7822e46618b","documentation_url":"https://developer.github.com/v3/repos/statuses/"}, setting build result to FAILURE

mac tests on failed with known kokoro/mac/ruby issue: #12538

@apolcyn apolcyn merged commit aafa875 into grpc:master Sep 19, 2017
apolcyn added a commit that referenced this pull request Sep 21, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants