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

feat(common): support injecting custom headers #13829

Merged
merged 5 commits into from
Mar 22, 2024

Conversation

scotthart
Copy link
Member

@scotthart scotthart commented Mar 22, 2024

Adds support for adding custom headers for both gRPC and REST.


This change is Reviewable

* An option to inject custom headers into the request.
*
* For REST endpoints, these headers are added to the HTTP headers. For gRPC
* endpoints, these headers are added to the `grpc::ClientContext` metadata.
Copy link
Member

Choose a reason for hiding this comment

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

add: @ingroup options which I am noticing I forgot to do for EnableServerRetriesOption.

Comment on lines 409 to 410
if (options.has<google::cloud::CustomHeadersOption>()) {
for (auto const& h : options.get<google::cloud::CustomHeadersOption>()) {
Copy link
Member

Choose a reason for hiding this comment

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

Drop the options.has<> conditional?

Also, s/google::cloud::CustomHeadersOption/CustomHeadersOption/

Comment on lines 403 to 404
if (options.has<google::cloud::CustomHeadersOption>()) {
for (auto const& h : options.get<google::cloud::CustomHeadersOption>()) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -406,6 +406,11 @@ void $metadata_class_name$::SetMetadata(grpc::ClientContext& context,
}
auto const& authority = options.get<AuthorityOption>();
if (!authority.empty()) context.set_authority(authority);
if (options.has<google::cloud::CustomHeadersOption>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: It looks like we're a little inconsistent with our qualifying. Options a few lines up, for example, is mentioned without google::cloud::.

@@ -406,6 +406,11 @@ void $metadata_class_name$::SetMetadata(grpc::ClientContext& context,
}
auto const& authority = options.get<AuthorityOption>();
if (!authority.empty()) context.set_authority(authority);
if (options.has<google::cloud::CustomHeadersOption>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: You could do without the has<T>() test (and hence the double lookup) in this case, at the cost of a once-only allocation of an empty map.

Copy link
Member Author

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 427 files reviewed, 5 unresolved discussions (waiting on @dbolduc and @devbww)

a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

add: @ingroup options which I am noticing I forgot to do for EnableServerRetriesOption.

Added for all options in this file missing the tag.


a discussion (no related file):

Previously, devbww (Bradley White) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

Optional nit: It looks like we're a little inconsistent with our qualifying. Options a few lines up, for example, is mentioned without google::cloud::.

Made qualification consistent across all options in the function.


a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

Drop the options.has<> conditional?

Also, s/google::cloud::CustomHeadersOption/CustomHeadersOption/

Done.


a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

ditto

Done.


a discussion (no related file):

Previously, devbww (Bradley White) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

Optional nit: You could do without the has<T>() test (and hence the double lookup) in this case, at the cost of a once-only allocation of an empty map.

Done.


@scotthart scotthart enabled auto-merge (squash) March 22, 2024 22:38
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 85.07463% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 93.04%. Comparing base (a6b575a) to head (50c15c9).
Report is 2 commits behind head on main.

Files Patch % Lines
...ternal/golden_rest_only_rest_metadata_decorator.cc 0.00% 6 Missing ⚠️
...olden/v1/internal/request_id_metadata_decorator.cc 0.00% 2 Missing ⚠️
.../internal/golden_thing_admin_metadata_decorator.cc 50.00% 1 Missing ⚠️
...rnal/golden_thing_admin_rest_metadata_decorator.cc 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13829      +/-   ##
==========================================
- Coverage   93.64%   93.04%   -0.61%     
==========================================
  Files        2258     2174      -84     
  Lines      195012   184802   -10210     
==========================================
- Hits       182620   171945   -10675     
- Misses      12392    12857     +465     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scotthart scotthart merged commit adf8c27 into googleapis:main Mar 22, 2024
62 of 63 checks passed
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