Fix #1228 - Improves webhooks to send labels at once#1566
Merged
miketaylr merged 9 commits intowebcompat:masterfrom May 24, 2017
Merged
Fix #1228 - Improves webhooks to send labels at once#1566miketaylr merged 9 commits intowebcompat:masterfrom
miketaylr merged 9 commits intowebcompat:masterfrom
Conversation
- We use the LOCALHOST key for testing - Takes this opportunity to straighten a bit the way we create import.
We might want to do that in the future but for now, it is basically unused code.
The goal is to make the function more usable in web hooks tests. We now have a function to check and one to compute a signature. It makes also fixtures independent with regards to content change.
Only one case is not tested for now, when basically we are communicating with GitHub. There are a couple of strategies for mocking the service. It needs a bit more cycles of thinking.
- Removes the GET. Flask handles this already for us - Creates only 1 requests for setting labels instead of two, probably avoiding race condition mentioned in webcompat#1553 - Sends plain/text messages where it is granted. - Logs the failure when tagging didn’t happen properly - Renames set_label as set_labels and rationalises the functions
miketaylr
approved these changes
May 23, 2017
Member
miketaylr
left a comment
miketaylr
left a comment
There was a problem hiding this comment.
LGTM. Just a few minor questions -- thanks Karl!
tests/test_webhook.py
Outdated
|
|
||
| # Any request that depends on parsing HTTP Headers (basically anything | ||
| # on the index route, will need to include the following: environ_base=headers | ||
| headers = {'HTTP_USER_AGENT': ('Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; ' |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
webcompat/webhooks/helpers.py
Outdated
| } | ||
| uri = 'https://api.github.com/repos/{0}/{1}/labels'.format( | ||
| app.config['ISSUES_REPO_URI'], issue_number) | ||
| return requests.post(uri, data=json.dumps(payload), headers=headers) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Member
Author
|
@miketaylr Modifications done. |
Member
|
Thanks Karl. Let's merge. |
miketaylr
added
in progress
status: add to changelog
and removed
in progress
status: add to changelog
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
r? @miketaylr
Instead of sending two requests which might have created the issue #1553, we are now sending one request only.
We also created a bunch of additional tests in their own file.
Comments in each commit have additional details explaining the context.
You can test locally the Webhook by doing.
Testing
or for just testing the webhook.