googleapis / gax-ruby

Google API Extensions for Ruby
https://rubygems.org/gems/google-gax
BSD 3-Clause "New" or "Revised" License
20 stars 22 forks source link

Make headers on operations client injectable #200

Closed fables-tales closed 5 years ago

fables-tales commented 5 years ago

For Google Ads we send additional headers for LRO reloads, specifically:

developer-token
login-customer-id

without these headers being injectable it's not possible for us to complete LRO calls successfully. This patch exposes headers in the constructor of the operations client so that we can pass these when we construct our operations client objects.

codecov[bot] commented 5 years ago

Codecov Report

Merging #200 into 1.x will decrease coverage by 0.04%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.x     #200      +/-   ##
==========================================
- Coverage   99.29%   99.24%   -0.05%     
==========================================
  Files          19       19              
  Lines        2123     2123              
==========================================
- Hits         2108     2107       -1     
- Misses         15       16       +1
Impacted Files Coverage Δ
spec/google/gax/bundling_spec.rb 99.71% <0%> (-0.29%) :arrow_down:

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 c4460b5...963e391. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #200 into 1.x will decrease coverage by 0.04%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.x     #200      +/-   ##
==========================================
- Coverage   99.29%   99.24%   -0.05%     
==========================================
  Files          19       19              
  Lines        2123     2123              
==========================================
- Hits         2108     2107       -1     
- Misses         15       16       +1
Impacted Files Coverage Δ
spec/google/gax/bundling_spec.rb 99.71% <0%> (-0.29%) :arrow_down:

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 c4460b5...963e391. Read the comment docs.

dazuma commented 5 years ago

@samphippen Just to be clear, this is for the current gapic generated clients, not the new generator, right? Do you have a similar requirement for the new generator?

fables-tales commented 5 years ago

@dazuma yes, we would need this in the new generator, but we need this for this generator right now for our LROs to work.

fables-tales commented 5 years ago

I dug in to this further, and I think there's a bug with per-rpc headers: https://github.com/googleapis/gax-ruby/blob/1.x/lib/google/gax/settings.rb#L128

This line of code

metadata = (metadata.dup if metadata) || {}

is attempting to use the metadata attr_accessor on the Settings instance. However, in ruby, creating a local variable implicitly binds it to nil in the body of the block:

p foo
foo = 3

will print nil for example. This local binding also beats methods of the same name. As such metadata on that line will always reference the local variable metadata which will be bound to nil, and as such this entire expression will always assign the empty hash from the || {} part of the line.

blowmage commented 5 years ago

Yeah, I can confirm that line is not working as expected. It looks like changing that line to metadata = @metadata.dup || {} would fix it.

blowmage commented 5 years ago

Closing in favor of #205.