Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

Retry request when Github API returns status 404 #191

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions highfive/newpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import gzip
import re
import os
from retry import retry

from highfive import irc, payload

Expand Down Expand Up @@ -47,6 +48,9 @@
class UnsupportedRepoError(IOError):
pass

class HttpClientError(Exception):
pass

class HighfiveHandler(object):
def __init__(self, payload):
self.payload = payload
Expand Down Expand Up @@ -102,6 +106,7 @@ def api_req(self, method, url, data=None, media_type=None):
body = f.read()
return { "header": header, "body": body }

@retry(HttpClientError, delay=1, backoff=2, max_delay=3)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I am clear from the documentation of the retry package, this will allow up to three attempts: the initial attempt, another one second after that fails, and another two seconds after the second attempt fails.

It would likely be easier for others to read if we just used the tries argument and drop max_delay.

def set_assignee(self, assignee, owner, repo, issue, user, author, to_mention):
try:
self.api_req(
Expand All @@ -111,6 +116,8 @@ def set_assignee(self, assignee, owner, repo, issue, user, author, to_mention):
except urllib2.HTTPError, e:
if e.code == 201:
pass
elif e.code == 400:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on #190, these should all probably be handling for the 404 case, not a 400 case.

raise HttpClientError
else:
raise e

Expand Down Expand Up @@ -146,6 +153,7 @@ def get_irc_nick(self, gh_name):

return None

@retry(HttpClientError, delay=1, backoff=2, max_delay=3)
def is_collaborator(self, commenter, owner, repo):
"""Returns True if `commenter` is a collaborator in the repo."""
try:
Expand All @@ -156,6 +164,8 @@ def is_collaborator(self, commenter, owner, repo):
except urllib2.HTTPError, e:
if e.code == 404:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use retry here, it will change the behavior of this method when all of the attempts fail. It would be nice if retry could return a value we provide on the final failure.

elif e.code == 400:
raise HttpClientError
else:
raise e

Expand All @@ -172,6 +182,7 @@ def post_warnings(self, diff, owner, repo, issue):
if warnings:
self.post_comment(warning_summary % '\n'.join(map(lambda x: '* ' + x, warnings)), owner, repo, issue)

@retry(HttpClientError, delay=1, backoff=2, max_delay=3)
def post_comment(self, body, owner, repo, issue):
try:
self.api_req(
Expand All @@ -180,6 +191,8 @@ def post_comment(self, body, owner, repo, issue):
except urllib2.HTTPError, e:
if e.code == 201:
pass
elif e.code == 400:
raise HttpClientError
else:
raise e

Expand Down Expand Up @@ -211,6 +224,7 @@ def unexpected_branch(self):
return (expected_target, actual_target) \
if expected_target != actual_target else False

@retry(HttpClientError, delay=1, backoff=2, max_delay=3)
def is_new_contributor(self, username, owner, repo):
# If this is a fork, we do not treat anyone as a new user. This is
# because the API endpoint called in this function indicates all
Expand All @@ -227,6 +241,8 @@ def is_new_contributor(self, username, owner, repo):
except urllib2.HTTPError, e:
if e.code == 422:
return True
elif e.code == 400:
raise HttpClientError
else:
raise e

Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@
author_email='[email protected]',
url='https://github.com/rust-lang-nursery/highfive',
packages=['highfive'],
install_requires=['retry'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this project has aimed to avoid adding external dependencies in the past, but that may be in my head. Pinging @nrc, if he has any feelings about this.

)