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

removed requests as required dependency #128

Merged
merged 1 commit into from Jul 11, 2021
Merged

Conversation

tom-pytel
Copy link
Contributor

@tom-pytel tom-pytel commented Jul 11, 2021

@wu-sheng wu-sheng added this to the 0.7.0 milestone Jul 11, 2021
@tom-pytel
Copy link
Contributor Author

So if you have nothing against it I will look into replacing requests with standard python lib urllib to do the connections?

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Do we need

@tom-pytel tom-pytel removed the request for review from kezhenxu94 July 11, 2021 15:21
@wu-sheng
Copy link
Member

FYI @potiuk This has been reverted and kept optional only.

@tom-pytel
Copy link
Contributor Author

Do we need

A different communication method? If requests is license problematic then why not change to standard python lib if possible, one less dependency too.

@wu-sheng
Copy link
Member

Do we need

A different communication method? If requests is license problematic then why not change to standard python lib if possible, one less dependency too. psf/requests#5797. We just need their new release.

Sorry, it was a copy/paste issue. There is no issue. This dependency issue should be resolved from the upstream repo already.

@wu-sheng wu-sheng merged commit edb26c0 into apache:master Jul 11, 2021
@tom-pytel
Copy link
Contributor Author

Heads up, I just grabbed the latest master which should have this and #127 merged and this change is not present. setup.py still contains requests as a required dependency. Guessing merge with @127 list it?

@kezhenxu94
Copy link
Member

kezhenxu94 commented Jul 12, 2021

Hey, I can see requests is not required dependency. https://github.com/apache/skywalking-python/blob/master/setup.py It’s in http extra dependency.

Sorry, you are right, mixup on my part.

@potiuk
Copy link
Member

potiuk commented Jul 12, 2021

Correct. I mentioned that in my report. So this is right, it's not necesssary according to the ASF Policy (similarly in Airflow we have a lot of extras with requests) - the "optional" LGPL dependency is acceptable. But once the new version is out, I think it would be great to make it "minimal" version of requests - this way the LGPL dependency becomes optional even for optional dependencies ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants