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 bug with httpx usage - ensure basic auth is set when provided #285

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

Conversation

c0llab0rat0r
Copy link
Contributor

@c0llab0rat0r c0llab0rat0r commented Jun 14, 2021

  • Fix a defect in the httpx proxy class implementation
  • Add test proving it works - nginx is configured in front of IPFS and set to require Basic authentication.

requests session will pass authentication parameters on every request, but httpx does not. This could be a defect in httpx, based on a review of the httpx code.

Because Docker Hub has recently established request rate limits on pulls, pytest skip logic was added to prevent the test from executing when run with Travis. This could be changed if an open source special account was created with Docker Hub, and credentials from that account configured in Travis.

There's also logic to skip the test if run on Windows, because there isn't currently an IPFS docker image that supports Windows.

@c0llab0rat0r c0llab0rat0r changed the title [wip] Fix bug with httpx usage - ensure basic auth is set when provided Fix bug with httpx usage - ensure basic auth is set when provided Jun 14, 2021
@c0llab0rat0r
Copy link
Contributor Author

@ntninja this is ready to look at.

@ntninja
Copy link
Contributor

ntninja commented Jun 16, 2021

This will take some time…

@c0llab0rat0r
Copy link
Contributor Author

c0llab0rat0r commented Jun 29, 2021

This will take some time…

Any feedback yet?

I'd like this to be in 0.8.0.

@c0llab0rat0r
Copy link
Contributor Author

Discussed with @ntninja on chat.

Will look into using pytest-localserver as an alternative to docker-compose to avoid adding an additional dependency.

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