Skip to content

Conversation

@scotthart
Copy link
Member

@scotthart scotthart commented Feb 23, 2022

fixes #8288


This change is Reviewable

@scotthart scotthart requested a review from a team as a code owner February 23, 2022 23:46
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: f7d08a2419ca5e151182610cd53d95e931ee59b2

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #8445 (7b67f1e) into main (df6fa36) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8445      +/-   ##
==========================================
- Coverage   95.09%   95.09%   -0.01%     
==========================================
  Files        1367     1367              
  Lines      122501   122520      +19     
==========================================
+ Hits       116491   116508      +17     
- Misses       6010     6012       +2     
Impacted Files Coverage Δ
...oud/internal/oauth2_authorized_user_credentials.cc 100.00% <100.00%> (ø)
...loud/internal/oauth2_compute_engine_credentials.cc 100.00% <100.00%> (ø)
.../internal/oauth2_refreshing_credentials_wrapper.cc 100.00% <100.00%> (ø)
...rnal/oauth2_refreshing_credentials_wrapper_test.cc 100.00% <100.00%> (ø)
...oud/internal/oauth2_service_account_credentials.cc 97.32% <100.00%> (ø)
google/cloud/bigtable/internal/common_client.h 94.02% <0.00%> (-5.98%) ⬇️
google/cloud/storage/internal/curl_handle.h 80.00% <0.00%> (-2.86%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 97.75% <0.00%> (-0.25%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.02% <0.00%> (-0.08%) ⬇️
.../cloud/storage/benchmarks/throughput_experiment.cc 74.87% <0.00%> (+0.50%) ⬆️
... and 3 more

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 df6fa36...7b67f1e. Read the comment docs.

@scotthart scotthart enabled auto-merge (squash) February 24, 2022 00:06
@coryan coryan disabled auto-merge February 24, 2022 13:49
RefreshingCredentialsWrapper::RefreshingCredentialsWrapper(
CurrentTimeFn current_time_fn)
: current_time_fn_(std::move(current_time_fn)) {}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, it seems that the now parameter becomes redundant? Your call.

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 6 files reviewed, 1 unresolved discussion (waiting on @coryan)


google/cloud/internal/oauth2_refreshing_credentials_wrapper.cc, line 26 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Thanks, it seems that the now parameter becomes redundant? Your call.

Removed the now parameter and just calling current_time_fn where needed.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 7b67f1e3b98f5961ad7142b55f6118596723354e

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @scotthart)

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @scotthart)

@scotthart scotthart merged commit d8c41eb into googleapis:main Feb 25, 2022
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.

Add functor for updated time in RefreshingCredentialsWrapper in lieu of now().

3 participants