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

Added support for more WKT #816

Merged
merged 7 commits into from
Nov 19, 2018

Conversation

mayankcpdixit
Copy link
Contributor

@mayankcpdixit mayankcpdixit commented Nov 17, 2018

Adding support for more well known types.
Adding support for wrappers (google.protbuf.wrappers)
Adding Example for using StringValue protobuf wrapper in url template option

Should fix: #808

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! I don't think we need to add the greeter server or greeter proxy since we can't run those in CI (at least not right now). Otherwise looks good!

@mayankcpdixit
Copy link
Contributor Author

@johanbrandhorst Right. Removed those.

@johanbrandhorst
Copy link
Collaborator

It looks like the generated files don't build, can you see if we can fix the signature of the new functions?

@johanbrandhorst
Copy link
Collaborator

This is great, thanks! I will merge once the build has finished successfully :).

@johanbrandhorst
Copy link
Collaborator

Oops, looks like the Bazel files are out of date: https://circleci.com/gh/grpc-ecosystem/grpc-gateway/736. Could you run the following command to update everything please?

$ docker run -itv $(pwd):/grpc-gateway -w /grpc-gateway --entrypoint /bin/bash --rm \
    l.gcr.io/google/bazel -c 'bazel run :gazelle_fix; bazel run :buildifier'

From https://github.com/grpc-ecosystem/grpc-gateway/blob/master/CONTRIBUTING.md#i-want-to-regenerate-the-files-after-making-changes.

@johanbrandhorst
Copy link
Collaborator

On a sidenote: @drigz, how often will unrelated user PRs be failing like this? Is there some way we can just update these manually and not have it fail the build?

@drigz drigz mentioned this pull request Nov 19, 2018
@drigz
Copy link
Contributor

drigz commented Nov 19, 2018

@johanbrandhorst Created #817 to avoid noise on this PR.

@johanbrandhorst
Copy link
Collaborator

That bazel fix command is really impressive, wow! I'll merge this once the build finishes :).

@codecov-io
Copy link

Codecov Report

Merging #816 into master will decrease coverage by 0.4%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
- Coverage    53.3%   52.89%   -0.41%     
==========================================
  Files          30       30              
  Lines        3371     3397      +26     
==========================================
  Hits         1797     1797              
- Misses       1399     1425      +26     
  Partials      175      175
Impacted Files Coverage Δ
protoc-gen-grpc-gateway/descriptor/types.go 41.08% <ø> (ø) ⬆️
runtime/convert.go 13.75% <0%> (-2.67%) ⬇️

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 09679ff...56f3478. Read the comment docs.

@mayankcpdixit
Copy link
Contributor Author

That bazel fix command is really impressive, wow! I'll merge this once the build finishes :).

Agree. It finished smoothly though it took really long. I felt there should be easier way so I don't have to download heavy docker images. Maybe use multistage build. idk. just thinking out loud.

@johanbrandhorst johanbrandhorst merged commit a73f992 into grpc-ecosystem:master Nov 19, 2018
@johanbrandhorst
Copy link
Collaborator

Thank you for your contribution!

@mayankcpdixit
Copy link
Contributor Author

No problem @johanbrandhorst . I'm using it for one of my work item. So I had to make it work anyway.

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* Added support for more WKT

* Lint fix: added comments on exported functions

* Lint fix: added comments on exported functions

* Added end to end example to show support of StringValue in url option template

* updated more example protos; removed server and proxy for now

* Updated convert.go to fix float and other wkt

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

Successfully merging this pull request may close these issues.

Unable to add protobuf wrappers in url template option
5 participants