kombu icon indicating copy to clipboard operation
kombu copied to clipboard

Hotfix: pycurl again

Open spawn-guy opened this issue 8 months ago • 7 comments

as discussed in https://github.com/celery/kombu/issues/2258

  • remove urllib3 http client (as not ready for production)
  • bring back pycurl as hard sqs dependency
  • fix curl client request.body bytes conversion (as in current code)

spawn-guy avatar Apr 07 '25 09:04 spawn-guy

Codecov Report

Attention: Patch coverage is 59.79899% with 80 lines in your changes missing coverage. Please review.

Project coverage is 81.06%. Comparing base (1645812) to head (1e8ee2f).

Files with missing lines Patch % Lines
kombu/asynchronous/http/curl.py 59.39% 65 Missing and 15 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2284      +/-   ##
==========================================
- Coverage   81.55%   81.06%   -0.49%     
==========================================
  Files          77       77              
  Lines        9541     9628      +87     
  Branches     1162     1179      +17     
==========================================
+ Hits         7781     7805      +24     
- Misses       1568     1621      +53     
- Partials      192      202      +10     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Apr 07 '25 09:04 codecov[bot]

how this is different then #2261?

@auvipy mostly other fixes that were done are left in place

  • enabled/fixed SQS Secure connection (which is False before and is a security issue, IMHO). https://github.com/celery/kombu/pull/2261/files#diff-addef505aa79f47faf5306b836fee7c232031bc1a43a725d4e969d876d6e7f6fR95
  • refactored SSL-Certificate finding for AWS Connection, as in in boto3 https://github.com/celery/kombu/pull/2261/files#diff-6c205d7cb977e240bd95cea4910cf6dfcdd400e4a458e74eb2e90386453ad27eL9 test: https://github.com/celery/kombu/pull/2261/files#diff-511ee8556160dd2a68c72a5f2e7313694aac8b94550e9976497788dc39410e5aL93
  • Client returns BaseClient type instead of CurlClient (this opens up future addition of different clients without a rewrite) https://github.com/celery/kombu/pull/2261/files#diff-9aa4b3dd9022d46a6295f9b51e8ce167a0c71fd1b5ab60cda1eed480af710229R15
  • add docblock https://github.com/celery/kombu/pull/2261/files#diff-0b9dc15666563c75234a40db6965f23696190ca3f78eaaee75dc4e2a66e16a01L239
  • fixed pycurl request.body bytes encoding that was introduced later (as reported in the test of experimental branch)

spawn-guy avatar Apr 08 '25 10:04 spawn-guy

I can also port certifi ssl certificate finding mechanism to pycurl implementation if desired

spawn-guy avatar Apr 08 '25 10:04 spawn-guy

@auvipy explain "clean revert"

spawn-guy avatar Apr 17 '25 09:04 spawn-guy

@auvipy i've been thinking about your request. and i got to a conclusion. i assume you want me to "create a revert-commit" and then apply the other fixes that would be removed by the "revert commit".

i see no point in doing exactly that. only for the sake of "commit history". and the thing is - the PR's seem to be "squash-merged" that means all commits are squashed into 1 (leaving commit messages in the description only).

so, in the end .diff will be exactly the same as in the current PR code. (revert and re-apply, or apply new on top of existing code)

spawn-guy avatar Apr 23 '25 14:04 spawn-guy

before merging this, we should also consider https://github.com/celery/kombu/pull/2300

auvipy avatar May 17 '25 08:05 auvipy

@auvipy @Nusnus will you be so kind to rebase these branches onto main instead of merging into them?

spawn-guy avatar Jun 10 '25 09:06 spawn-guy