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

fix(storage): support per-request options #11445

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Apr 29, 2023

With the rest_internal::RestClient implementation we were not respecting some per-request options. This changes the implementation of rest_internal::RestClient to use the options provided in rest_internal::RestContext. That seemed cleaner than using the options span.

I gave myself some leeway by adding default constructors to some storage::internal::*Request classes. It makes it easier to write unit tests where the contents of the request are uninteresting.

I included an integration test in rest_internal::RestClient.

Fixes #11441


This change is Reviewable

With the `rest_internal::RestClient` implementation we were not
respecting some per-request options. This changes the implementation of
`rest_internal::RestClient` to use the options provided in
`rest_internal::RestContext`. That seemed cleaner than using the options
span.

I gave myself some leeway by adding default constructors to some
`storage::internal::*Request` classes. It makes it easier to write unit
tests where the contents of the request are uninteresting.

I included an integration test in `rest_internal::RestClient`.
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Apr 29, 2023
@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (90a6882) 93.78% compared to head (290774a) 93.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11445      +/-   ##
==========================================
+ Coverage   93.78%   93.80%   +0.02%     
==========================================
  Files        1805     1805              
  Lines      162337   162930     +593     
==========================================
+ Hits       152244   152838     +594     
+ Misses      10093    10092       -1     
Impacted Files Coverage Δ
google/cloud/internal/curl_rest_client.h 100.00% <ø> (ø)
google/cloud/storage/internal/bucket_requests.h 100.00% <ø> (ø)
google/cloud/storage/internal/object_requests.h 97.50% <ø> (-0.04%) ⬇️
google/cloud/internal/curl_rest_client.cc 96.35% <100.00%> (+0.16%) ⬆️
...loud/internal/curl_rest_client_integration_test.cc 98.80% <100.00%> (+0.07%) ⬆️
google/cloud/internal/rest_context.h 100.00% <100.00%> (ø)
...oogle/cloud/storage/internal/bucket_acl_requests.h 100.00% <100.00%> (ø)
google/cloud/storage/internal/bucket_requests.cc 99.51% <100.00%> (+<0.01%) ⬆️
...oud/storage/internal/default_object_acl_requests.h 100.00% <100.00%> (ø)
google/cloud/storage/internal/hmac_key_requests.h 100.00% <100.00%> (ø)
... and 5 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@coryan coryan marked this pull request as ready for review April 29, 2023 20:22
@coryan coryan requested a review from a team as a code owner April 29, 2023 20:22
@coryan coryan merged commit 971a4e4 into googleapis:main May 1, 2023
@coryan coryan deleted the fix-storage-propagate-options-through-rest-client branch May 1, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RestClient ignores user-agent option
2 participants