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

Fix TCPConnection for verify_ssl = false #33

Merged
merged 1 commit into from
Jul 31, 2018

Conversation

bpicolo
Copy link
Contributor

@bpicolo bpicolo commented Jul 30, 2018

This fixes #32

I didn't see any relevant tests but am happy to add any test that you think should be here - unfamiliar with working in this library so I wouldn't know what makes the most sense off hand : )

@codecov-io
Copy link

codecov-io commented Jul 30, 2018

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #33   +/-   ##
=======================================
  Coverage   93.51%   93.51%           
=======================================
  Files          16       16           
  Lines        1003     1003           
=======================================
  Hits          938      938           
  Misses         65       65

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 6673e8d...23ad462. Read the comment docs.

Copy link
Owner

@tomplus tomplus left a comment

Choose a reason for hiding this comment

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

As you can see in the preamble of this file, it is auto-generated by swagger-codegen. We shouldn't edit it directly but fix the generator and regenerate this library. It'll took some time so to not block you to use this library, I will merge it for the time being.

Thanks for your contribution!

@@ -6,7 +6,7 @@
No description provided (generated by Swagger Codegen https://github.com/swagger-api/swagger-codegen) # noqa: E501

OpenAPI spec version: v1.10.1

Copy link
Owner

Choose a reason for hiding this comment

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

Could you rollback this unnecessary change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah sure, that was just editor auto trimming trailing whitespace

ssl_context=ssl_context,
verify_ssl=configuration.verify_ssl
ssl_context=ssl_context if configuration.verify_ssl else None,
verify_ssl=configuration.verify_ssl,
Copy link
Owner

Choose a reason for hiding this comment

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

... and this , too. Small and precise change-set is simpler to read and apply. Thank you.

@bpicolo bpicolo force-pushed the 32_fix_verify_ssl_false branch from 9bd2203 to 23ad462 Compare July 30, 2018 23:35
@bpicolo
Copy link
Contributor Author

bpicolo commented Jul 30, 2018

I must admit I totally missed that comment. I will also go upstream in swagger codegen - thanks for being open to this PR regardless (track that here: swagger-api/swagger-codegen#8507)

@tomplus
Copy link
Owner

tomplus commented Jul 31, 2018

LGTM

@tomplus tomplus merged commit ba7bbe2 into tomplus:master Jul 31, 2018
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.

aiohttp client constructor is non-functional if verify_ssl=False is used
3 participants