Summary

Fix various aspects of dispatching webhook with auth credentials

Review Request #11171 — Created Sept. 14, 2020 and discarded Sept. 21, 2020, 12:40 a.m.

Information

Owner

yxejamir

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Fix error dispatching webhook with auth credentials

The error was:

AttributeError: 'SplitResult' object has no attribute 'params'

The 5-tuple used for rebuilding the URL was missing the fragment element instead.

Log webhook server responses on error

If the exception from urlopen represents an HTTP response, then the included
information may be useful to the site admin in troubleshooting.

Always send Authorization header on authenticated webhooks

The problem with HTTPBasicAuthHandler is that it will first attempt to make an
unauthenticated request, and will retry with authentication only if a 401
response was seen. However, some servers respond with 403 if the right
credentials are not supplied right away.

Testing Done

This is a forward-ported version of my patches on top of 3.0.18, which is powering our production site.

Note: It is best to review the changes one commit at a time. The patch series is available publicly on Github.

Issues

Description	From	Last Updated
From the description, it looks like there's multiple independent changes all being introduced. Can you split this into a review …	chipx86	Sept. 21, 2020, 12:37 a.m.
E501 line too long (81 > 79 characters)	reviewbot	Sept. 14, 2020, 5:17 a.m.
Can you reorder the new import to be in alphabetical order? Helps us keep our imports organized.	chipx86	Sept. 21, 2020, 12:37 a.m.
Now that the list of imports here is short, will it fit on one line?	chipx86	Sept. 21, 2020, 12:37 a.m.
This can be placed earlier up, in alphabetical order.	chipx86	Sept. 21, 2020, 12:39 a.m.
We prefer a blank line between statements and new blocks (helps keep blocks from running together, avoiding issues with, say, …	chipx86	Sept. 21, 2020, 12:40 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/notifications/webhooks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```

Change Summary:

Fix line length issue.

Commit:

afbe8ae314a8ccdc2c0781601d4ce6489bdbdaaf

c529b3770fe08b2aad97975552b65c5d30417a46

Diff:

Revision 2 (+11 -14)

Show changes

reviewboard/notifications/webhooks.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

From the description, it looks like there's multiple independent changes all being introduced. Can you split this into a review request per change, so we can more easily get these in and keep them separate?

yxejamir Sept. 21, 2020, 12:40 a.m.

No problem. I will have to figure out later, what the best way is to submit a patch series for review, like you would with git send-email <rev-list options>.

reviewboard/notifications/webhooks.py (Diff revision 2)

The issue has been resolved. Show all issues

Can you reorder the new import to be in alphabetical order? Helps us keep our imports organized.

reviewboard/notifications/webhooks.py (Diff revision 2)

The issue has been resolved. Show all issues

Now that the list of imports here is short, will it fit on one line?

yxejamir Sept. 21, 2020, 12:40 a.m.

As a side note: for new projects I would consider introducing Black into the development workflow.

reviewboard/notifications/webhooks.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This can be placed earlier up, in alphabetical order.
```

reviewboard/notifications/webhooks.py (Diff revision 2)

The issue has been resolved. Show all issues

We prefer a blank line between statements and new blocks (helps keep blocks from running together, avoiding issues with, say, two if blocks appearing as part of the same set of conditionals).

Status:: Discarded