-
Notifications
You must be signed in to change notification settings - Fork 150
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 path rules, add basic auth #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patches. Pushed patch #1 with a tweak. I added some comments on the other two.
Do you know if this is a bugzilla feature, or is it something that's implemented site specific? Since this seems to be a general HTTP feature I'm trying to figure out if we should bake it into the API like this, or instead expose the transport object to the API user to tweak as needed.
API won't work at all if you cannot access the URL at first place. So this is a general feature to at all get to the XML-RPC API part. If your Bugzilla API is behind that auth, then you will always get HTTP 401 error for unauthorised URL, which renders this library unusable. |
Any other issues you still have? |
Your patch layout is making this series difficult to review. Patch #4 should be squashed into patch #1. Patch #3 should be squashed into patch #2. Patch #5 seems like it was added mid review which makes this series a moving target. Please split out your style change patches and submit them separately. Also indicate what tool you are using that flags these bits, because our pycodestyle and pylint configs aren't flagging anything. We can keep this PR for tracking your auth additions |
Is anything else I can help you? |
Thank you for the contribution. The patch still had a stray newline, long line, and the 'import requests' movement which were unrelated to the patch. I adjusted all those and pushed. Please confirm git master still works correctly |
@isbm FYI, upstream I removed the basic_auth support, in favor of allowing API users to pass in their own |
This PR rewrites path rules in a standard way, using
urlparse/urlunparse
as well as adds sometimes required Basic authentication via headers, so the XML-RPC API URL can be at all accessed before even calling any functions.Example usage: