fix: Tokens in config are no longer ignored when there are tokens in the environment, part of refactoring in preparation for app token refreshing #284
Conversation
TrishGillett
force-pushed
the
token-manager-2
branch
7 times, most recently
from
ec6dbe0
to
78918b9
Compare
for more information, see https://pre-commit.ci
TrishGillett
force-pushed
the
token-manager-2
branch
from
6459b3d
to
4d49dd0
Compare
TrishGillett
changed the title
TrishGillett
changed the title
There was a problem hiding this comment.
This looks great @TrishGillett 🎉 . Just a nit about idiomatic pytest.
edgarrmondragon
changed the title
TrishGillett
force-pushed
the
token-manager-2
branch
from
209e311
to
0aa5771
Compare
for more information, see https://pre-commit.ci
|
| token_manager = AppTokenManager("12345;;key\\ncontent") | ||
| assert token_manager.github_app_id == "12345" | ||
| assert token_manager.github_private_key == "key\ncontent" | ||
| assert token_manager.github_installation_id == "" |
There was a problem hiding this comment.
I think that
tap-github/tap_github/authenticator.py
Line 132 in aade338
""
I think we either want to parse this as empty string and then change the other code to look for "falsy" or we want to parse this as "None" and then we do not need to change the code.
I have been getting
404 Client Error: Not Found for url: https://api.github.com/app/installations//access_tokens
post this change
There was a problem hiding this comment.
Hey @camerondavison, thanks for reporting and sorry this change caused an issue. Taking a look now. 👀
There was a problem hiding this comment.
Put up a PR to fix this here, if you like you can try pegging your version of the tap to the commit 38e1bdf9381c3a3fa17cceada9e1ede7be57ae6b to see if it fixes your issue.
There was a problem hiding this comment.
I've also tagged v1.4.8
…#291) Addresses the problem described by @camerondavison [here](#284 (comment)), and introduced in #284. If not provided, installation ID should be set to None, since that is the way `generate_app_access_token` expects a missing installation ID to be represented.
This PR continues the work started in #281, completing the major refactor. This is accomplished by creating PersonalTokenManager and AppTokenManager classes, moving logic to them, and adding tests.
After this PR, I'll be able to easily implement refreshing for app tokens and other app token related features.
In additional to the added unit tests, I have tested running an extractor on top of the proposed code, in a couple scenarios:
I am not easily able to test the case of personal tokens being detected from the environment, just because changing the name of environment variables is not simple in my set up.