Closed jakob-keller closed 5 months ago
Attention: Patch coverage is 96.96970%
with 1 lines
in your changes are missing coverage. Please review.
Project coverage is 86.29%. Comparing base (
c74c796
) to head (704508c
).
Files | Patch % | Lines |
---|---|---|
aiobotocore/httpsession.py | 96.87% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
sorry for delay! Kids got hand foot mouth sequentially and I've been working on getting our condo back on the market due to tenant destruction (pain) with no money (they didn't pay). Will get to this ASAP.
sorry for delay! Kids got hand foot mouth sequentially and I've been working on getting our condo back on the market due to tenant destruction (pain) with no money (they didn't pay). Will get to this ASAP.
Ouch, I feel ya! Take your time!
gonna get this done today! (pep talking myself lol)
is it possible to use their proxy manager concept to make the logic more similar? would help to maintain going forwards.
is it possible to use their proxy manager concept to make the logic more similar? would help to maintain going forwards.
I don't see how. We don't use (proxy) managers at all.
We could create a ._get_session()
method that extracts relevant logic from .send()
, if that is what you propose. Should I give it a try?
I just rebased the PR onto main and added a commit that pulls out session creation into ._get_session()
. It makes the code a bit more readable, but introduces another round trip through the event loop, which could have a minor performance impact. Let me know, what you think. I can drop the commit easily.
ok looks great, we really need to add a proxy test later though. One suggestion, replace:
chunked = None
if headers_.get('Transfer-Encoding', '').lower() == 'chunked':
# aiohttp wants chunking as a param, and not a header
headers_.pop('Transfer-Encoding', '')
chunked = True
with something like this function after _get_session
def _chunked(self, headers):
transfer_encoding = headers.get('Transfer-Encoding', '')
if chunked := transfer_encoding.lower() == 'chunked':
# aiohttp wants chunking as a param, and not a header
headers.pop('Transfer-Encoding', '')
return chunked
so it matches botocore some more.
oh and then chunked=self._chunked(headers_),
thank you and sorry for the delays! finally finished the painting, now for all the other 1000 tiny things to fix lol
I have no idea, why CI is failing. This should be unrelated to this PR.
hmm:
> raise ParamValidationError(report=report.generate_report())
E botocore.exceptions.ParamValidationError: Parameter validation failed:
E Invalid type for parameter TopicArn, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
https://github.com/aio-libs/aiobotocore/actions/runs/8129847790/job/22217412080?pr=1077
hmm, also seeing an xml parse error, I wonder if it's moto that got upgraded
I have no idea, why CI is failing. This should be unrelated to this PR.
For some reason chunked=None is not equivalent to chunked=False. I just reverted to the old logic and now CI is passing.
thank you so much, sorry for delay!
Thanks for the thorough review! Much appreciated!
Description of Change
This PR fixes support for HTTPS proxies.
Assumptions
Connection to the HTTPS proxy and to the HTTPS endpoint may use a single
ssl.SSLContext
.Checklist for All Submissions
ssl.SSLContext
to be used both for connections to the HTTPS proxy as well as the HTTPS endpointChecklist when updating botocore and/or aiohttp versions