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.

Sign up for GitHub

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

Jump to bottom

feat: add timeout to options and use it in the reqOpts #455

Merged
merged 4 commits into from
Jul 31, 2019
Merged

feat: add timeout to options and use it in the reqOpts #455

merged 4 commits into from
Jul 31, 2019

Conversation

AVaksman
Copy link
Contributor

@AVaksman AVaksman commented Jul 29, 2019 •
edited
Loading

add timeout to service and pass it to reqOpts.
With this timeout can be exposed to users in the child dependencies (storage, bigquery...)
Will apply to googleapis/nodejs-storage/issues/733

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 29, 2019
@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #455 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   98.26%   98.27%   +<.01%     
==========================================
  Files           5        5              
  Lines         404      405       +1     
  Branches       99       99              
==========================================
+ Hits          397      398       +1     
  Misses          2        2              
  Partials        5        5
Impacted Files Coverage Δ
src/service.ts 96.42% <100%> (+0.06%) ⬆️

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 755635c...1af7c88. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 29, 2019 •
edited
Loading

Codecov Report

Merging #455 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   98.26%   98.27%   +<.01%     
==========================================
  Files           5        5              
  Lines         404      405       +1     
  Branches       99       99              
==========================================
+ Hits          397      398       +1     
  Misses          2        2              
  Partials        5        5
Impacted Files Coverage Δ
src/service.ts 96.42% <100%> (+0.06%) ⬆️

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 4ad0bb1...784ac6b. Read the comment docs.

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2019
JustinBeckwith
JustinBeckwith reviewed Jul 29, 2019
View reviewed changes
src/service.ts
@@ -84,6 +85,7 @@ export class Service {
authClient: GoogleAuth;
private getCredentials: {};
readonly apiEndpoint: string;
timeout: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be optional? Or are we setting it to a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am referencing this below

src/service.ts
@@ -69,6 +69,7 @@ export interface ServiceOptions extends GoogleAuthOptions {
promise?: PromiseConstructor;
email?: string;
token?: string;
timeout?: number; // http.request.options.timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexander-fenster I'm curious - do we expose a gRPC option for timeout in the GAPIC libraries?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly, but GAPIC libraries can pass any gRPC option by prefixing it with grpc. and it will be passed all the way down to gRPC.

src/service.ts
@@ -103,6 +105,7 @@ export class Service {
constructor(config: ServiceConfig, options: ServiceOptions = {}) {
this.baseUrl = config.baseUrl;
this.apiEndpoint = config.apiEndpoint;
this.timeout = options.timeout!;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be nervous using the ! here because it looks like it can actually be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If options.timeout is undefined the default value for timeout will be used later and not swapped with undefined here

@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2019
@AVaksman AVaksman marked this pull request as ready for review July 29, 2019 17:06
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 29, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 31, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 31, 2019
@AVaksman AVaksman merged commit 3a3a251 into googleapis:master Jul 31, 2019
AVaksman added a commit to AVaksman/nodejs-common that referenced this pull request Aug 5, 2019
@AVaksman
add timeout to options and use it in the reqOpts googleapis#455
6fdb26b
bcoe pushed a commit that referenced this pull request Aug 5, 2019
@AVaksman @bcoe
feat: adds timeout to options and use it in reqOpts (#455)
90a6097
@stephenplusplus stephenplusplus mentioned this pull request Aug 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-fenster alexander-fenster alexander-fenster left review comments

@bcoe bcoe bcoe approved these changes

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AVaksman @bcoe @JustinBeckwith @alexander-fenster @googlebot @kokoro-team