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

apiv2: timeout #3006

Merged
merged 10 commits into from Jan 8, 2021
Merged

apiv2: timeout #3006

merged 10 commits into from Jan 8, 2021

Conversation

bkendall
Copy link
Contributor

@bkendall bkendall commented Jan 8, 2021

Description

This introduces a new timeout option to apiv2. This can (and should) be used rather than providing signal controllers and managing that logic in other places.

I updated an existing spot (proxying traffic through Hosting emulator) that used a signal to use the timeout instead. I also updated the project management API for listing projects to use the new timeout mechanism in the new apiv2 (this ended up requiring updating a bunch of tests - and for kicks, nock - but I think it was worth it).

Interesting decision I made: I did not expose timeout on the verb methods. Looking at the type definitions, I couldn't convince myself that VerbOptions was correct. Maybe putting it in ClientHandlingOptions would work (since that ends up being an extension of other options), but I wasn't thrilled with that since it's an option of the request, not the handling of it. Up for opinions, but it's not too much of a hassle to change later either.

Scenarios Tested

  • Making a request to the Hosting emulator proxying to a function that takes too long, when set with a hardcoded shorter timeout.
  • Tested projects:list with a hardcoded short timeout to see the failure.

@bkendall bkendall added the cleanup: request PRs for removing the request module from the CLI label Jan 8, 2021
@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Jan 8, 2021
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 65.427% when pulling 88bc357 on bk-apiv2-timeout into 8082b68 on master.

@bkendall bkendall merged commit 706ce19 into master Jan 8, 2021
@bkendall bkendall deleted the bk-apiv2-timeout branch January 8, 2021 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA. cleanup: request PRs for removing the request module from the CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants