Use a github access token stored in ndb. (#3132)
* Use a github access token stored in ndb. * Select client tokens based on least recent failure. * Add unit tests.
This commit is contained in:
Jason Robbins
GitHub
Родитель
7e58b19848
Коммит
4850e7cd9d
|
|
@ -82,3 +82,42 @@ def get_session_secret():
|
|||
"""Return the session secret key."""
|
||||
singleton = Secrets._get_or_make_singleton()
|
||||
return singleton.session_secret
|
||||
|
||||
|
||||
GITHUB_API_NAME = 'github'
|
||||
|
||||
|
||||
class ApiCredential(ndb.Model):
|
||||
"""A server-side-only list of values that we use to access APIs."""
|
||||
api_name = ndb.StringProperty(required=True)
|
||||
token = ndb.StringProperty()
|
||||
failure_timestamp = ndb.IntegerProperty(default=0)
|
||||
|
||||
@classmethod
|
||||
def select_token_for_api(cls, api_name: str) -> 'ApiCredential':
|
||||
"""Return one of our credientials for the requested API or make a blank."""
|
||||
query = ApiCredential.query(ApiCredential.api_name == api_name)
|
||||
all_for_api = query.fetch(None)
|
||||
if not all_for_api:
|
||||
blank_entry = ApiCredential(api_name=api_name)
|
||||
blank_entry.put()
|
||||
logging.info('Created an ApiCredential for %r', api_name)
|
||||
logging.info('Please use the Cloud Console to fill in a token')
|
||||
return blank_entry
|
||||
|
||||
# If ndb has multiple tokens for the same API, choose one
|
||||
# that has not failed recently for any reason (including quota).
|
||||
logging.info('Found %r tokens for %r', len(all_for_api), api_name)
|
||||
sorted_for_api = sorted(all_for_api, key=lambda ac: ac.failure_timestamp)
|
||||
return sorted_for_api[0]
|
||||
|
||||
@classmethod
|
||||
def get_github_credendial(cls) -> 'ApiCredential':
|
||||
"""Return an ApiCredential for GitHub."""
|
||||
return cls.select_token_for_api(GITHUB_API_NAME)
|
||||
|
||||
def record_failure(self, now=None) -> None:
|
||||
"""Mark this ApiCredential as failing now."""
|
||||
logging.info('Recording failure at %r', now or int(time.time()))
|
||||
self.failure_timestamp = now or int(time.time())
|
||||
self.put()
|
||||
|
|
|
|||
|
|
@ -82,3 +82,83 @@ class SecretsTest(testing_config.CustomTestCase):
|
|||
mock_make_random_key.assert_called_once_with()
|
||||
self.assertEqual(singleton2.xsrf_secret, 'old secret field')
|
||||
self.assertEqual(singleton2.session_secret, 'fake new random')
|
||||
|
||||
|
||||
class ApiCredentialTest(testing_config.CustomTestCase):
|
||||
|
||||
def tearDown(self):
|
||||
for old_entity in secrets.ApiCredential.query():
|
||||
old_entity.key.delete()
|
||||
|
||||
def test_select_token_for_api__first_use(self):
|
||||
"""When there are no credientials for an API, it makes one."""
|
||||
actual = secrets.ApiCredential.select_token_for_api('foo')
|
||||
|
||||
self.assertEqual('foo', actual.api_name)
|
||||
self.assertIsNone(actual.token)
|
||||
self.assertEqual(0, actual.failure_timestamp)
|
||||
self.assertEqual(
|
||||
1, len(list(secrets.ApiCredential.query())))
|
||||
|
||||
def test_select_token_for_api__one_exists(self):
|
||||
"""When there are no credientials for an API, it makes one."""
|
||||
foo_cred = secrets.ApiCredential(api_name='foo', token='token')
|
||||
foo_cred.put()
|
||||
|
||||
actual = secrets.ApiCredential.select_token_for_api('foo')
|
||||
|
||||
self.assertEqual('foo', actual.api_name)
|
||||
self.assertEqual('token', actual.token)
|
||||
self.assertEqual(0, actual.failure_timestamp)
|
||||
self.assertEqual(
|
||||
1, len(list(secrets.ApiCredential.query())))
|
||||
|
||||
def test_select_token_for_api__three_exist(self):
|
||||
"""When there are credientials, choose the earliest failure."""
|
||||
cred_2 = secrets.ApiCredential(
|
||||
api_name='foo', token='token 2', failure_timestamp=2)
|
||||
cred_2.put()
|
||||
cred_1 = secrets.ApiCredential(
|
||||
api_name='foo', token='token 1', failure_timestamp=1)
|
||||
cred_1.put()
|
||||
cred_3 = secrets.ApiCredential(
|
||||
api_name='foo', token='token 3', failure_timestamp=3)
|
||||
cred_3.put()
|
||||
|
||||
actual = secrets.ApiCredential.select_token_for_api('foo')
|
||||
|
||||
self.assertEqual('foo', actual.api_name)
|
||||
self.assertEqual('token 1', actual.token)
|
||||
self.assertEqual(1, actual.failure_timestamp)
|
||||
self.assertEqual(
|
||||
3, len(list(secrets.ApiCredential.query())))
|
||||
|
||||
def test_get_github_credendial(self):
|
||||
"""We can get a github token."""
|
||||
gh_cred = secrets.ApiCredential(
|
||||
api_name=secrets.GITHUB_API_NAME, token='hash')
|
||||
gh_cred.put()
|
||||
|
||||
actual = secrets.ApiCredential.get_github_credendial()
|
||||
|
||||
self.assertEqual('github', actual.api_name)
|
||||
self.assertEqual('hash', actual.token)
|
||||
self.assertEqual(0, actual.failure_timestamp)
|
||||
self.assertEqual(
|
||||
1, len(list(secrets.ApiCredential.query())))
|
||||
|
||||
def test_record_failure(self):
|
||||
"""We can record the time of failure for an API token."""
|
||||
NOW = 1234567890
|
||||
gh_cred = secrets.ApiCredential(
|
||||
api_name=secrets.GITHUB_API_NAME, token='hash')
|
||||
gh_cred.put()
|
||||
|
||||
gh_cred.record_failure(now=NOW)
|
||||
|
||||
updated_cred = list(secrets.ApiCredential.query())[0]
|
||||
self.assertEqual('github', updated_cred.api_name)
|
||||
self.assertEqual('hash', updated_cred.token)
|
||||
self.assertEqual(NOW, updated_cred.failure_timestamp)
|
||||
self.assertEqual(
|
||||
1, len(list(secrets.ApiCredential.query())))
|
||||
|
|
|
|||
|
|
@ -24,7 +24,11 @@ from urllib.error import HTTPError
|
|||
from urllib.parse import urlparse
|
||||
import base64
|
||||
|
||||
github_api_client = GhApi()
|
||||
from framework import secrets
|
||||
|
||||
|
||||
github_crediential = None
|
||||
github_api_client = None
|
||||
|
||||
LINK_TYPE_CHROMIUM_BUG = 'chromium_bug'
|
||||
LINK_TYPE_GITHUB_ISSUE = 'github_issue'
|
||||
|
|
@ -41,6 +45,24 @@ LINK_TYPES_REGEX = {
|
|||
URL_REGEX = re.compile(r'(https?://\S+)')
|
||||
|
||||
|
||||
def get_github_api_client():
|
||||
"""Set up the GitHub client."""
|
||||
global github_credential
|
||||
global github_api_client
|
||||
if github_api_client is None:
|
||||
github_credential = secrets.ApiCredential.get_github_credendial()
|
||||
github_api_client = GhApi(token=github_credential.token)
|
||||
|
||||
return github_api_client
|
||||
|
||||
|
||||
def rotate_github_client():
|
||||
"""Try a different github client, e.g., after quota is used up."""
|
||||
global github_api_client
|
||||
github_credential.record_failure()
|
||||
github_api_client = None # A new one will be selected on when needed.
|
||||
|
||||
|
||||
class Link():
|
||||
|
||||
@classmethod
|
||||
|
|
@ -71,17 +93,15 @@ class Link():
|
|||
self.is_error = False
|
||||
self.information = None
|
||||
|
||||
def _parse_github_markdown(self) -> dict[str, object]:
|
||||
parsed_url = urlparse(self.url)
|
||||
path = parsed_url.path
|
||||
owner = path.split('/')[1]
|
||||
repo = path.split('/')[2]
|
||||
ref = path.split('/')[4]
|
||||
file_path = '/'.join(path.split('/')[5:])
|
||||
def _fetch_github_file(
|
||||
self, owner: str, repo: str, ref: str, file_path: str,
|
||||
retries=1):
|
||||
"""Get a file from GitHub."""
|
||||
client = get_github_api_client()
|
||||
try:
|
||||
# try to get the branch information, if it exists, update branch name
|
||||
# this handles the case where the branch is renamed
|
||||
branch_information = github_api_client.repos.get_branch(
|
||||
# this handles the case where the branch is renamed.
|
||||
branch_information = client.repos.get_branch(
|
||||
owner=owner, repo=repo, branch=ref)
|
||||
ref = branch_information.name
|
||||
except HTTPError as e:
|
||||
|
|
@ -89,8 +109,28 @@ class Link():
|
|||
if e.code != 404:
|
||||
raise e
|
||||
|
||||
information = github_api_client.repos.get_content(
|
||||
try:
|
||||
information = client.repos.get_content(
|
||||
owner=owner, repo=repo, path=file_path, ref=ref)
|
||||
return information
|
||||
except HTTPError as e:
|
||||
logging.info(f'Got http response code {e.code}')
|
||||
if e.code != 404 and retries > 0:
|
||||
rotate_github_client()
|
||||
return self._fetch_github_file(
|
||||
owner, repo, ref, file_path, retries=retries - 1)
|
||||
else:
|
||||
raise e
|
||||
|
||||
def _parse_github_markdown(self) -> dict[str, object]:
|
||||
parsed_url = urlparse(self.url)
|
||||
path = parsed_url.path
|
||||
owner = path.split('/')[1]
|
||||
repo = path.split('/')[2]
|
||||
ref = path.split('/')[4]
|
||||
file_path = '/'.join(path.split('/')[5:])
|
||||
|
||||
information = self._fetch_github_file(owner, repo, ref, file_path)
|
||||
|
||||
# decode the content from base64
|
||||
content_str = information.content
|
||||
|
|
@ -101,7 +141,24 @@ class Link():
|
|||
|
||||
return information
|
||||
|
||||
def _parse_github_issue(self) -> dict[str, object]:
|
||||
def _fetch_github_issue(
|
||||
self, owner: str, repo: str, issue_id: int,
|
||||
retries=1) -> dict[str, Any]:
|
||||
"""Get an issue from GitHub."""
|
||||
try:
|
||||
client = get_github_api_client()
|
||||
resp = client.issues.get(owner=owner, repo=repo, issue_number=issue_id)
|
||||
return resp
|
||||
except HTTPError as e:
|
||||
logging.info(f'Got http response code {e.code}')
|
||||
if e.code != 404 and retries > 0:
|
||||
rotate_github_client()
|
||||
return self._fetch_github_issue(
|
||||
owner, repo, issue_id, retries=retries - 1)
|
||||
else:
|
||||
raise e
|
||||
|
||||
def _parse_github_issue(self) -> dict[str, str | None]:
|
||||
"""Parse the information from the github issue tracker."""
|
||||
|
||||
parsed_url = urlparse(self.url)
|
||||
|
|
@ -110,18 +167,17 @@ class Link():
|
|||
repo = path.split('/')[2]
|
||||
issue_id = path.split('/')[4]
|
||||
|
||||
resp = github_api_client.issues.get(
|
||||
owner=owner, repo=repo, issue_number=int(issue_id))
|
||||
resp = self._fetch_github_issue(owner, repo, int(issue_id))
|
||||
information = {
|
||||
'url': resp.get('url'),
|
||||
'number': resp.get('number'),
|
||||
'title': resp.get('title'),
|
||||
'user_login': (
|
||||
resp.get('user').get('login') if resp.get('user') else None),
|
||||
resp['user'].get('login') if resp.get('user') else None),
|
||||
'state': resp.get('state'),
|
||||
'state_reason': resp.get('state_reason'),
|
||||
'assignee_login': (
|
||||
resp.get('assignee').get('login') if resp.get('assignee') else None),
|
||||
resp['assignee'].get('login') if resp.get('assignee') else None),
|
||||
'created_at': resp.get('created_at'),
|
||||
'updated_at': resp.get('updated_at'),
|
||||
'closed_at': resp.get('closed_at'),
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ from internals.link_helpers import (
|
|||
|
||||
|
||||
class LinkHelperTest(testing_config.CustomTestCase):
|
||||
|
||||
def test_extract_urls_from_value(self):
|
||||
field_value = "https://www.chromestatus.com/feature/1234"
|
||||
urls = Link.extract_urls_from_value(field_value)
|
||||
|
|
@ -32,7 +33,7 @@ class LinkHelperTest(testing_config.CustomTestCase):
|
|||
field_value = "leadinghttps:https://www.chromestatus.com/feature/1234, https://www.chromestatus.com/feature/5678 is valid"
|
||||
urls = Link.extract_urls_from_value(field_value)
|
||||
self.assertEqual(urls, ["https://www.chromestatus.com/feature/1234", "https://www.chromestatus.com/feature/5678"])
|
||||
|
||||
|
||||
field_value = ["https://www.chromestatus.com/feature/1234", "not a valid urlhttps://www.chromestatus.com/feature/", None]
|
||||
urls = Link.extract_urls_from_value(field_value)
|
||||
self.assertEqual(urls, ["https://www.chromestatus.com/feature/1234"])
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче