Repository navigation
fix: skip commits rejected by GitHub with HTTP 422 - #539
iamshravanijadhav wants to merge 1 commit into
Conversation
Signed-off-by: Shravani <jadhavshravani223@gmail.com>
2d85678 to
99df626
Compare
| # Try to get the login from the commit sha | ||
| if login == None or login == "": | ||
| login = get_login_with_commit_hash(logger, auth, contributor, repo_id) | ||
| if login is None or login == "": | ||
| try: | ||
| login = get_login_with_commit_hash( | ||
| logger, auth, contributor, repo_id | ||
| ) | ||
| except httpx.HTTPStatusError as e: | ||
| if e.response.status_code == 422: | ||
| logger.warning( | ||
| f"Skipping commit {contributor.get('hash')} " | ||
| f"because GitHub returned HTTP 422." | ||
| ) | ||
| continue | ||
| raise |
There was a problem hiding this comment.
ok, given the context surrounding this fix, it may be a better approach to find a different commit from that contributor (which is plumbing we don't fully have in place yet).
I can see potentially skipping the contributor for now though, especially if we add the above as a comment in hthis area of the code.
| if isinstance(exception, httpx.HTTPStatusError) and exception.response.status_code == 422: | ||
| return False |
There was a problem hiding this comment.
it may be a better fit with the existing code in this part of the codebase to create a new ResourceUnprocessable exception that we throw when the status is 422 and then include that in the list of exceptions not to retry here.
| logger, auth, commit_data, repo_id=1 | ||
| ) | ||
|
|
||
| assert exc_info.value.response.status_code == 422 |
There was a problem hiding this comment.
is there a chance exc_info might not exist in this scope? maybe its worth indenting the assertions
| def test_422_response_is_not_retried(self, gda): | ||
| assert not GithubDataAccess._decide_retry_policy( | ||
| httpx.HTTPStatusError( | ||
| "422 Unprocessable Entity", | ||
| request=Mock(), | ||
| response=Mock(status_code=422), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This function is inside the TestEndpointUrl class. It should be moved to a top level function like the other tests added in this PR (or better yet, put in its own TestResponseErrors class to group the tests togethern
github-project-automation
Bot
moved this to In Testing
in CollectOSS + 8Knot WG Feature Roadmap
|
Thank you for the review! I'll work on the suggested changes, update the exception handling and tests, and add the comment explaining the current approach. |
2 participants
Description
This PR fixes the HTTP 422 error that occurs when a commit is no longer available in the repository.
This PR fixes #526
Notes for Reviewers
The fix skips commits that return HTTP 422 and continues processing the remaining commits. I tested the changes in Docker, and all 9 tests passed.
-Added handling for HTTP 422 errors.
-Stopped retrying requests when GitHub returns HTTP 422.
-Added logging when a commit lookup fails with HTTP 422.
-Added tests for the retry policy and commit lookup error handling.
Signed commits
Generative AI disclosure
Please select one option:
If AI tools were used, please provide details below:
- What tools were used? Chatgpt
- How were these tools used? I used ChatGPT to understand the HTTP 422 issue and debug it.
- Did you review these outputs before submitting this PR? Yes, I reviewed the changes and ran the tests.