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 exception in Urllib3 when dealing with filelike body. #1399

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

isra17
Copy link
Contributor

@isra17 isra17 commented Oct 23, 2022

Description

Only use len on body if it's not a filelike object.

Fixex #1235

How Has This Been Tested?

tox -e test-instrumentation-urllib3

@isra17 isra17 requested a review from a team October 23, 2022 17:33
@isra17 isra17 force-pushed the isra/io-body branch 2 times, most recently from 5432fbb to 0054033 Compare October 24, 2022 13:35
@isra17 isra17 force-pushed the isra/io-body branch 3 times, most recently from 75a29c2 to d83e23e Compare October 29, 2022 15:56
@ocelotl
Copy link
Contributor

ocelotl commented Nov 18, 2022

Please resolve conflicts

@isra17
Copy link
Contributor Author

isra17 commented Nov 18, 2022

Done

@isra17 isra17 force-pushed the isra/io-body branch 5 times, most recently from 6ac4a43 to 3655586 Compare November 21, 2022 20:59
@srikanthccv
Copy link
Member

Please fix the conflicts

@isra17
Copy link
Contributor Author

isra17 commented Feb 13, 2023

Done again.

@srikanthccv srikanthccv enabled auto-merge (squash) February 13, 2023 18:46
auto-merge was automatically disabled February 13, 2023 19:14

Head branch was pushed to by a user without write access

@isra17
Copy link
Contributor Author

isra17 commented Feb 13, 2023

Seems like the test broke since the first commit. Pushed a fixed version.

@gtmanfred
Copy link

@shalevr @srikanthccv any chance this could be merged? it is preventing us from migrating from opentracing to opentelemetry.

@srikanthccv
Copy link
Member

It can't be merged unless all the requirements are met. The author didn't let maintainers make edits, which prevents me from updating (out-of-date with base) and merging it.

@isra17
Copy link
Contributor Author

isra17 commented Feb 24, 2023

It can't be merged unless all the requirements are met. The author didn't let maintainers make edits, which prevents me from updating (out-of-date with base) and merging it.

I can't give edit access to the PR since the fork is being owned by an org and not a personnal account. Let me rebase again and fix conflict. If there's conflict again in the future I would suggest you just cherry-pick my commit and open your own pull request. I understand maintaining open-source software is thankless work (And I thank you for otel!), but this PR have many times the amount of work it actually required due to at least 5 rebase with conflict that appears each time right before merging.

@isra17 isra17 force-pushed the isra/io-body branch 2 times, most recently from eb8ef9b to cad55c3 Compare February 24, 2023 22:43
@isra17
Copy link
Contributor Author

isra17 commented Feb 24, 2023

My rebase conflicted with a7bd563 which did a similar thing than my PR (moved all the test to test_urllib3_metrcs.py, but changed the test to use HttpTestBase while my PR worked on this comment #1399 (comment) and pretty much rewrite the whole test suite. I kept my rewrite instead of the master version, however I can't use the HTTP server since it only support GET request with a specific payload while my tests requires more use-cases.

@isra17 isra17 force-pushed the isra/io-body branch 4 times, most recently from 614addb to aefab5b Compare February 24, 2023 23:53
@isra17
Copy link
Contributor Author

isra17 commented Feb 24, 2023

Ok, finally fixed the test with the new base utils assert_.... Should be green now!

@srikanthccv
Copy link
Member

It can't be merged unless all the requirements are met. The author didn't let maintainers make edits, which prevents me from updating (out-of-date with base) and merging it.

I can't give edit access to the PR since the fork is being owned by an org and not a personnal account. Let me rebase again and fix conflict. If there's conflict again in the future I would suggest you just cherry-pick my commit and open your own pull request. I understand maintaining open-source software is thankless work (And I thank you for otel!), but this PR have many times the amount of work it actually required due to at least 5 rebase with conflict that appears each time right before merging.

I wouldn't mind somebody else creating the PR for the same fix, getting that merged, and then closing this. You may have n number of reasons not to give edit access, even if it wasn't an org account. I mentioned that because the last time I wanted to merge this two weeks ago, this was not up to date with the base branch. Many PRs in this repo let the maintainer make edits (for such cases) and merge quickly. I also enabled the auto-merge, but that didn't help either. I can't hold other PRs ready to merge because this needs rebasing etc...

@srikanthccv srikanthccv enabled auto-merge (squash) February 25, 2023 00:39
@shalevr
Copy link
Member

shalevr commented Feb 25, 2023

LGTM, thank you

@gtmanfred
Copy link

gtmanfred commented Feb 25, 2023

Thank you very much all of yall! we really appreciate it.

@srikanthccv srikanthccv merged commit 85ae95c into open-telemetry:main Feb 25, 2023
pridhi-arora pushed a commit to pridhi-arora/opentelemetry-python-contrib that referenced this pull request Feb 25, 2023
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.

5 participants