-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: pass agent options when using agent pool config #149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
==========================================
+ Coverage 73.48% 73.95% +0.47%
==========================================
Files 2 2
Lines 396 407 +11
Branches 41 40 -1
==========================================
+ Hits 291 301 +10
Misses 104 104
- Partials 1 2 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, if @JustinBeckwith's comments about deferred loading are addressed.
Have you tested manually end to end (since we unfortunately don't have a system test suite).
@bcoe i did not test manually end-to-end (e.g. |
Manual regression testing was performed against Note in the expandable log output below, the "using linked agent with options" output is not in this PR, but was a console logging entry used for illustrative purposes during testing. Additionally, some unnecessary information was redacted. Evidence for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming it works everywhere we expect it to 🙃
pool
as used in@google-cloud/common
: https://github.com/googleapis/nodejs-common/blob/v3.0.0/src/util.ts#L37-L40pool
are passed through to the underlying agent, only if the agent is to be createdpool
is now a "protected" member, only to make test cases possible given the singleton~ish variable bleedforever
tests that were supposed to testhttps
, but actually were testinghttp
Fixes #148 🦕