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

[python] add socket_options to configuration for the rest client #7795

Merged
merged 3 commits into from
Oct 28, 2020

Conversation

meadsteve
Copy link
Contributor

This PR adds a property to the configuration object for the generated code rest client called socket_options. This property is passed down into urllibs3's ProxyManager/PoolManager as addition_pool_args. This extra key word argument should be picked up by the underlying HTTPConnection objects.

This enables consumers of the generated client to configuring the underlying connection if needed. In my specific use case I need this to set some keepalive settings for the connections.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @spacether

@spacether
Copy link
Contributor

closing and re-opening to kick off ci again

@spacether spacether closed this Oct 24, 2020
@spacether spacether reopened this Oct 24, 2020
@spacether
Copy link
Contributor

spacether commented Oct 24, 2020

@meadsteve this looks great, thank you!
Would you be willing to add a test asserting that the socket options are being used by the PoolManager?
In the past we have had features break because no tests were ever written verifying that a feature existed, and someone tweaked code and broke something. Adding a test would protect this feature in the future.
A good place to put a test is this folder.

@meadsteve
Copy link
Contributor Author

Thanks. The tests sound good but I wasn't sure where to start so thanks for the suggestion.
The build still looks like it's failing but I'm not sure it's something I've done?

@spacether
Copy link
Contributor

Yup, that CI error is unrelated to your PR; please ignore it.

…t client

This mirrors work done on the python generator
@meadsteve
Copy link
Contributor Author

@spacether I've added a test to cover this in python_experimental. I couldn't see any existing structure to do this for the non experimental python. Let me know if there's anything else you'd expect to see before this gets merged.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

This looks great!
Thank you for the PR and for adding the tests!

@spacether spacether merged commit ea2cdd2 into OpenAPITools:master Oct 28, 2020
@spacether spacether added this to the 5.0.0 milestone Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants