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

added drop in replacement for requests #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

added drop in replacement for requests #10

wants to merge 3 commits into from

Conversation

clayshieh
Copy link

Added file from my repo here which works as a drop in replacement for requests.

@joohoi
Copy link
Owner

joohoi commented Jun 21, 2018

I would prefer integrating (only) the required parts to the auth hoook file itself in order to make the installation more convenient - a get-one-file-and-get-going experience. I would also like to give credit where it's due, so I believe replacing the line "Requires Certbot >= 0.10, Python 2.7 and requests library." in README.md with something like:

Requires Certbot >= 0.10 and Python.

HTTP connectivity is provided by a vendored library [https://github.com/clayshieh/requests_stdlib](https://github.com/clayshieh/requests_stdlib).

would be in order.

@joohoi
Copy link
Owner

joohoi commented Jun 21, 2018

This would also give the benefit of not having to have import requests line in the code, which might confuse someone reading the code later on, as it's not apparent that the functionality is provided by something else than the actual python-requests.

I am aware that this requires a bit of refactoring in the actual hook file itself. If you don't feel comfortable doing so, I can do it (with your permission to vendor the library obviously).

@clayshieh
Copy link
Author

Not a problem!

I made the necessary changes but don't have a acme dns instance to test to see if everything still works. The part Im worried about is the register_account function. You don't specify the Content-Type header like you do in the other functions and you use json.dumps on the dictionary object. I checked requests' behavior for this case and it omits adding the Content-Type header but urllib2 will by default assign application/x-www-form-urlencoded to any post request that has data in it.

@jvanasco
Copy link

this honestly seems like a silly idea. certbot requires the requests library, so there is no real advantage to removing it as a dependency and replacing it with code that is not as mature.

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.

3 participants