API: fix GitHub access tests #134
Conversation
GitHub no longer accepts user password authenitcation for its APIs, so we now use personal access tokens to make API requests.
src/punx/github_handler.py
Outdated
|
|
||
| uname, pwd = open(creds_file_name, 'r').read().strip().split() | ||
| return dict(user=uname, password=pwd) | ||
| return open(creds_file_name, 'r').read().strip().split()[0] |
There was a problem hiding this comment.
I need to keep the credentials file to contain: github_email password token with any whitespace separator. As written, this returns github_email which is then used below as token.
There was a problem hiding this comment.
this file is used by multiple software, needs to keep same structure
There was a problem hiding this comment.
The file can keep its structure but, to return the token, this function must return item the last item ([-1]) from the list. Will this also work with your own version of the file that only contains the token?
src/punx/github_handler.py
Outdated
| @@ -59,45 +59,45 @@ | |||
|
|
|||
| def get_BasicAuth_credentials(creds_file_name = None): | |||
There was a problem hiding this comment.
This function gets the GitHub credentials but they are no longer BasicAuth (username & password). I suggest changing the name to get_GitHub_credentials.
There was a problem hiding this comment.
Request function name be changed.
src/punx/github_handler.py
Outdated
| :param str repo_name: name of repository in https://github.com/nexusformat (default: *definitions*) | ||
| :returns bool: True if using GitHub credentials | ||
| """ | ||
| repo_name = repo_name or self.appName | ||
|
|
||
|
|
||
| token = get_BasicAuth_credentials() if token is None else token |
There was a problem hiding this comment.
with above refactor so get_GitHub_credentials() returns a dict:
token = get_GitHub_credentials()["token"] if token is None else tokenThere was a problem hiding this comment.
Why should get_GitHub_credentials return a dictionary? Only the token is ever used.
There was a problem hiding this comment.
That's true within this software. But on my system, the exact same file is used in other contexts that need the additional content and formatting. A change here would compel revision to the other software.
Why should the current interface change?
There was a problem hiding this comment.
Do you import get_GitHub_credentials from punx to other packages?
src/punx/github_handler.py
Outdated
| @@ -158,15 +162,15 @@ def download(self): | |||
| #requests.packages.urllib3.disable_warnings(InsecureRequestWarning) | |||
| disable_warnings(InsecureRequestWarning) | |||
|
|
|||
| creds = get_BasicAuth_credentials() | |||
| token = get_BasicAuth_credentials() | |||
There was a problem hiding this comment.
op cit.
token = get_GitHub_credentials()["token"]
There was a problem hiding this comment.
@carterbox : Thanks for the PR! Changes requested as noted.
|
Also, once we are done with technical corrections and updates to make the code compliant with Python3, I intend to apply |
|
BTW, the commands to run the tests are listed in |
|
Ya, I just wanted feedback on my comments before I proceeded with any changes. Thanks! |
Fixes broken tests by updating GitHub authentication from username/password to personal token and by changing the branch pointer to "main" from "master".