-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Methods checking for 204 status handle 404 incorrectly #2760
Description
Context
I am trying to build some automation using PyGithub and a Github App to authenticate to our organization.
The token I can get has an expiration of 1 hour.
The idea of my workflow is to do the following steps:
- Get a token from Github App
- Create pull request
- Notify reviewers
- Wait for PR to be merged with a flexible timeout to allow reviewers time to review.
- Once PR is merged, do more steps
The problem arises in 4 if the timeout exceeds 1 hour.
Problem
In the scenario above I noticed that even after the authentication token has expired, calling pr.is_merged() returns False.
Looking at the code the behavior I was seeing makes sense:
def is_merged(self) -> bool:
"""
:calls: `GET /repos/{owner}/{repo}/pulls/{number}/merge <https://docs.github.com/en/rest/reference/pulls>`_
"""
status, headers, data = self._requester.requestJson("GET", f"{self.url}/merge")
return status == 204
since the status of the request with expired token would be 401.
I see two issues with this:
- False negatives - a PR could have been merged but
is_mergedreturns False. - Consumer is not notified in any way that their authentication has expired.
This also differs from other methods that I have been using like pr.get_commits().get_page(0) or pr.get_labels().get_page(0) which fail with the expected github.BadCredentialsException as soon as the token expires.
I also tested with a token that has no access to my organization, but in that case an expected 403 was raised which seems to be handled by github.GithubRetry.
Steps to Reproduce
- Create a Github Token on your account with
repoaccess - Use token to create a client and get a pull request
pr - Remove Github token from your account
4 . Runpr.is_merged.
For example:
import github
github_client = github.Github("<YOUR TOKEN>")
upstream_repo = github_client.get_repo("PyGithub/PyGithub")
pr = upstream_repo.get_pull(2757)
pr.is_merged()
False
pr.get_commits().get_page(0)
[Commit(sha="7569ac6c33e3682fa42ddf7aee1640198461a557"), Commit(sha="626a0435d063b12f9a9501a282e7371fe8783494")]
#
# Remove token from account
#
pr.is_merged()
False
pr.get_commits().get_page(0)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File ".../site-packages/github/PaginatedList.py", line 258, in get_page
headers, data = self.__requester.requestJsonAndCheck(
File ".../site-packages/github/Requester.py", line 442, in requestJsonAndCheck
return self.__check(
File ".../site-packages/github/Requester.py", line 487, in __check
raise self.__createException(status, responseHeaders, data)
github.GithubException.BadCredentialsException: 401 {"message": "Bad credentials", "documentation_url": "https://docs.github.com/rest"}
Workaround
I call pr.get_commits().get_page(0) as well to ensure I can catch the token expiration scenario and handle it.