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

stream_info: add setResponseCode and update local_reply to take a normal StreamInfo #14402

Merged
merged 3 commits into from
Dec 16, 2020

Conversation

esmet
Copy link
Contributor

@esmet esmet commented Dec 14, 2020

Commit Message: stream_info: add setResponseCode and update local_reply to take a normal StreamInfo
Additional Description: This makes local_reply a little cleaner by removing a small implementation detail and adding a new setter function to StreamInfo.
Risk Level: medium (refactors existing code)
Testing: No new tests
Docs Changes: None
Release Notes: None

…mal StreamInfo

Signed-off-by: John Esmet <john.esmet@gmail.com>
/**
* @param code the HTTP response code to set for this request.
*/
virtual void setResponseCode(uint32_t code) PURE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The callers of this function pass in Http::Code but the underlying type is uint32_t, so I went with that.

Open to suggestions as to whether we think this should remain uint32_t or change to Http::Code.

Signed-off-by: John Esmet <john.esmet@gmail.com>
@alyssawilk alyssawilk self-assigned this Dec 15, 2020
@alyssawilk
Copy link
Contributor

LG overall - want to take a look at CI and ping back?

@esmet
Copy link
Contributor Author

esmet commented Dec 15, 2020

LG overall - want to take a look at CI and ping back?

Yes it looks like I missed updating a test. Will do shortly and ping back.

Signed-off-by: John Esmet <john.esmet@gmail.com>
@esmet
Copy link
Contributor Author

esmet commented Dec 15, 2020

Pushed the fix and CI looks 💚

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

LGTM! @snowp want to take a look as well?

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@snowp snowp merged commit ffbf952 into envoyproxy:master Dec 16, 2020
@esmet esmet deleted the set-response-code branch December 16, 2020 00:34
mpuncel added a commit to mpuncel/envoy that referenced this pull request Dec 16, 2020
* master: (49 commits)
  sds: allow multiple init managers share sds target (envoyproxy#14357)
  [http] Remove legacy codecs (envoyproxy#14381)
  http2: Add integration tests for METADATA and RST_STREAM frame flood mitigation for upstream servers (envoyproxy#14365)
  test: start dissolving :printers_include rule. (envoyproxy#14429)
  integration tests: re-enable set_node_on_first_message_only (envoyproxy#14270)
  formatter: add a formatter that returns a google::protobuf::Struct rather than a string (envoyproxy#14258)
  ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service (envoyproxy#14189)
  deps: update protobuf to 3.14 (envoyproxy#14253)
  stream_info: add setResponseCode and update local_reply to take a normal StreamInfo (envoyproxy#14402)
  http: alpn upstream (envoyproxy#13922)
  Moved starttls integration test to test/extensions/transport_sockets/starttls. (envoyproxy#14425)
  generic conn pool: directly use thread local cluster (envoyproxy#14423)
  wasm: add mathetake to CODEOWNERS (envoyproxy#14427)
  wasm: clear route cache when modifying HTTP request headers. (envoyproxy#14318)
  tls: disable TLS inspector injection (envoyproxy#14404)
  aggregate cluster: cleanups (envoyproxy#14411)
  Mark starttls_integration_test flaky on Windows (envoyproxy#14419)
  tcp: improved unit testing (envoyproxy#14415)
  config: making protocol config explicit (envoyproxy#14362)
  wasm: dead code (envoyproxy#14407)
  ...

Signed-off-by: Michael Puncel <mpuncel@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