Skip to content

Conversation

spawn-guy
Copy link
Contributor

@spawn-guy spawn-guy commented Apr 7, 2025

as discussed in #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)

Copy link

codecov bot commented Apr 7, 2025

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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@Nusnus Nusnus self-requested a review April 7, 2025 09:29
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

how this is different then #2261?

@spawn-guy
Copy link
Contributor Author

spawn-guy commented Apr 8, 2025

how this is different then #2261?

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

@spawn-guy spawn-guy requested a review from auvipy April 8, 2025 10:23
@spawn-guy
Copy link
Contributor Author

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

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

I want this rebased after the clean revert

@spawn-guy
Copy link
Contributor Author

@auvipy explain "clean revert"

@spawn-guy
Copy link
Contributor Author

@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 spawn-guy requested a review from auvipy April 23, 2025 14:50
@auvipy
Copy link
Member

auvipy commented May 17, 2025

before merging this, we should also consider #2300

@spawn-guy
Copy link
Contributor Author

spawn-guy commented Jun 10, 2025

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

@@ -252,14 +252,19 @@ def _setup_request(self, curl, request, buffer, headers, _pycurl=pycurl):
setopt(meth, True)

if request.method in ('POST', 'PUT'):
body = request.body.encode('utf-8') if request.body else b''
Copy link
Member

Choose a reason for hiding this comment

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

you can rebase this so that we can merge this 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.

Id close this one completely and go with the other one with Urllib3.

Shall I?

Copy link
Member

Choose a reason for hiding this comment

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

that could be an option, but may be just extracting the pycurl related improvement would make it usable and the urllib3 vr PR easier to review :)

Copy link
Member

Choose a reason for hiding this comment

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

extracted #2322

@auvipy auvipy added this to the 5.6.0 milestone Jun 14, 2025
@spawn-guy spawn-guy closed this Jun 24, 2025
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.

2 participants