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

docs: improve gRPC-JSON filter doc #4184

Merged
merged 4 commits into from
Aug 17, 2018
Merged

Conversation

ctoomey
Copy link
Contributor

@ctoomey ctoomey commented Aug 16, 2018

Signed-off-by: ctoomey ctoomey@livongo.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: improve gRPC-JSON filter doc
Risk Level: Low
Testing: Viewed doc on github
Docs Changes: Added link to protobuf JSON mapping, added doc for how stream request/response params handled, added sample config
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: ctoomey <ctoomey@livongo.com>
@lizan lizan self-assigned this Aug 16, 2018
static_resources:
listeners:
- name: listener1
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this, listener2 can serve both gRPC and its RESTful JSON mapping.

Copy link
Contributor Author

@ctoomey ctoomey Aug 16, 2018

Choose a reason for hiding this comment

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

Good to know, didn't realize that. Will simplify the config.

Signed-off-by: ctoomey <ctoomey@livongo.com>
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks to taking up this! The sample LGTM, will defer to @mattklein123 or other maintainer for wordsmithing.

- name: envoy.grpc_json_transcoder
config:
proto_descriptor: "/tmp/envoy/proto.pb"
Copy link
Member

Choose a reason for hiding this comment

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

nit: 2 space indent

services: ["HelloWorld"]
print_options:
add_whitespace: true
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM other than @lizan comments. Thank you!

Signed-off-by: ctoomey <ctoomey@livongo.com>
@ctoomey
Copy link
Contributor Author

ctoomey commented Aug 17, 2018

Fixed config spacing

Signed-off-by: ctoomey <ctoomey@livongo.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mattklein123 mattklein123 merged commit 176500a into envoyproxy:master Aug 17, 2018
snowp pushed a commit to snowp/envoy that referenced this pull request Aug 20, 2018
* origin/master: (34 commits)
  fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189)
  docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194)
  fix sometimes returns response body to HEAD requests (envoyproxy#3985)
  admin: fix config dump order (envoyproxy#4197)
  thrift: allow translation between downstream and upstream protocols (envoyproxy#4136)
  Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120)
  upstream: allow extension protocol options to be used with http filters (envoyproxy#4165)
  [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169)
  docs: improve gRPC-JSON filter doc (envoyproxy#4184)
  stats: refactoring MetricImpl without strings (envoyproxy#4190)
  fuzz: coverage profile generation instructions. (envoyproxy#4193)
  upstream: rebuild cluster when health check config is changed (envoyproxy#4075)
  build: use clang-6.0. (envoyproxy#4168)
  thrift_proxy: introduce header transport (envoyproxy#4082)
  tcp: allow connection pool callers to store protocol state (envoyproxy#4131)
  configs: match latest API changes (envoyproxy#4185)
  Fix wrong mock function name. (envoyproxy#4187)
  Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182)
  Converting envoy configs to V2 (envoyproxy#2957)
  Add timestamp to HealthCheckEvent definition (envoyproxy#4119)
  ...

Signed-off-by: Snow Pettersen <snowp@squareup.com>
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