-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Improve GitHub authentication errors #24502
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
Changes from 5 commits
fbbe0d0
74c73af
5e1cb43
b0c0428
3694bac
f5574ff
bd54df1
4bc1db1
436c913
3317a0c
e3a1660
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Show actionable guidance when GitHub rejects ddev's configured token. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ | |
| from time import time | ||
| from typing import TYPE_CHECKING, overload | ||
|
|
||
| from ddev.utils.github_errors import GITHUB_AUTHENTICATION_STATUS_CODES, GitHubAuthenticationError | ||
|
|
||
| if TYPE_CHECKING: | ||
| from typing import Any, Literal | ||
|
|
||
|
|
@@ -305,7 +307,7 @@ def __api_get(self, *args, **kwargs): | |
| return self.__api_call('get', *args, **kwargs) | ||
|
|
||
| def __api_call(self, method, *args, **kwargs): | ||
| from httpx import HTTPError | ||
| from httpx import HTTPError, HTTPStatusError | ||
|
|
||
| retry_wait = 2 | ||
| while True: | ||
|
|
@@ -314,7 +316,7 @@ def __api_call(self, method, *args, **kwargs): | |
|
|
||
| # https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limiting | ||
| # https://docs.github.com/en/rest/guides/best-practices-for-integrators?apiVersion=2022-11-28#dealing-with-rate-limits | ||
| if response.status_code == 403 and response.headers['X-RateLimit-Remaining'] == '0': # noqa: PLR2004 | ||
| if response.status_code == 403 and response.headers.get('X-RateLimit-Remaining') == '0': # noqa: PLR2004 | ||
| self.__status.wait_for( | ||
| float(response.headers['X-RateLimit-Reset']) - time() + 1, | ||
| context='GitHub API rate limit reached', | ||
|
|
@@ -325,5 +327,10 @@ def __api_call(self, method, *args, **kwargs): | |
| retry_wait *= 2 | ||
| continue | ||
|
|
||
| response.raise_for_status() | ||
| try: | ||
| response.raise_for_status() | ||
| except HTTPStatusError as e: | ||
| if e.response.status_code in GITHUB_AUTHENTICATION_STATUS_CODES: | ||
| raise GitHubAuthenticationError.from_http_status_error(e) from e | ||
|
Comment on lines
+359
to
+360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This now maps every sync-client 403 that is not a primary-limit response ( Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in f5574ff. Secondary limits are now identified from either |
||
| raise | ||
| return response | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| import httpx | ||
| from pydantic import BaseModel, ConfigDict, Field | ||
|
|
||
| from ddev.utils.github_errors import GITHUB_AUTHENTICATION_STATUS_CODES, GitHubAuthenticationError | ||
| from ddev.utils.rate_limiting import NULL_SNAPSHOT, BudgetSnapshot, InstrumentedAsyncLimiter | ||
|
|
||
| from .defaults import default_github_rate_limiter | ||
|
|
@@ -236,8 +237,14 @@ async def _request( | |
| # the action. (Transport errors are never retried, and are not caught here: after | ||
| # one we cannot know whether the action executed.) Give up on the last attempt or | ||
| # on a non-rate-limit status, which waiting cannot fix. | ||
| if attempt == self._max_rate_limit_retries or not self._is_rate_limit_response(exc.response): | ||
| raise | ||
| is_rate_limit_response = self._is_rate_limit_response(exc.response) | ||
| if is_rate_limit_response: | ||
| if attempt == self._max_rate_limit_retries: | ||
| raise | ||
| continue | ||
| if exc.response.status_code in GITHUB_AUTHENTICATION_STATUS_CODES: | ||
| raise GitHubAuthenticationError.from_http_status_error(exc) from exc | ||
|
Comment on lines
+255
to
+256
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a side-effectful async call fails with 401/403, this conversion now raises Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in bd54df1 after follow-up. |
||
| raise | ||
| raise RuntimeError("unreachable: the retry loop always returns or raises") # pragma: no cover | ||
|
|
||
| async def _paginated_request( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # (C) Datadog, Inc. 2026-present | ||
| # All rights reserved | ||
| # Licensed under a 3-clause BSD style license (see LICENSE) | ||
| from __future__ import annotations | ||
|
|
||
| import httpx | ||
|
|
||
| GITHUB_AUTHENTICATION_STATUS_CODES = frozenset((401, 403)) | ||
|
|
||
|
|
||
| def github_authentication_error_message(status_code: int, *, action: str = 'requested operation') -> str: | ||
| """Return actionable guidance for a GitHub authentication failure.""" | ||
| return ( | ||
| f'GitHub denied the {action} (HTTP {status_code}). The configured token may be invalid, expired, ' | ||
| 'or missing required permissions. Run `ddev config set github.token` to configure a valid token.' | ||
| ) | ||
|
|
||
|
|
||
| class GitHubAuthenticationError(Exception): | ||
| """A GitHub HTTP failure caused by invalid authentication or insufficient permissions.""" | ||
|
|
||
| def __init__(self, message: str, *, request: httpx.Request, response: httpx.Response) -> None: | ||
| super().__init__(message) | ||
| self.request = request | ||
| self.response = response | ||
|
|
||
| @classmethod | ||
| def from_http_status_error(cls, error: httpx.HTTPStatusError) -> GitHubAuthenticationError: | ||
| """Build an authentication error while retaining the original HTTP context.""" | ||
| return cls( | ||
| github_authentication_error_message(error.response.status_code), | ||
| request=error.request, | ||
| response=error.response, | ||
| ) |
Uh oh!
There was an error while loading. Please reload this page.