-
Notifications
You must be signed in to change notification settings - Fork 75
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
make accessToken an io to use gh app's expiring tokens #567
Conversation
Codecov Report
@@ Coverage Diff @@
## master #567 +/- ##
===========================================
- Coverage 91.47% 74.17% -17.30%
===========================================
Files 24 25 +1
Lines 657 608 -49
Branches 2 2
===========================================
- Hits 601 451 -150
- Misses 56 157 +101
Continue to review full report at Codecov.
|
This reverts commit 0845bc9
making it a function so that GithubApp implementation could catch an expired token exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach 👍
Do the integration tests work locally?
Also I feel like we need to adapt the existing docs and create some more for someone looking to extend access token like you.
/** | ||
* Source of static or expiring github tokens | ||
*/ | ||
trait AccessTokens[F[_]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trait AccessTokens[F[_]] { | |
trait AccessToken[F[_]] { |
def withAccessToken[T](f: Option[String] => F[GHResponse[T]]): F[GHResponse[T]] | ||
} | ||
|
||
object AccessTokens {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object AccessTokens {} |
/** | ||
* A simple static version | ||
*/ | ||
class StaticAccessTokens[F[_]](accessToken: Option[String]) extends AccessTokens[F] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class StaticAccessTokens[F[_]](accessToken: Option[String]) extends AccessTokens[F] { | |
class StaticAccessToken[F[_]](accessToken: Option[String]) extends AccessToken[F] { |
* add doc
I've renamed AccessTkoens and added some docs, my english might not be perfect though. I tried running integrations tests locally with my personal token, and some of the tests fail for an obvious reason (me not having enough permissions for the github4s repository). Others are fine. |
|
||
object GithubAPIv3 { | ||
|
||
def noAuth[F[_]: Sync](client: Client[F], config: GithubConfig) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing return type
Co-authored-by: Ben Fradet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your great contribution 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Thanks for your time @reimai!
Hi. In my project I want to use Github App auth, which operates expiring tokens. So here is a proposed change to the insides of the api:
The default Github constructor stays the same, it's accessToken is just wrapped into a pure version of AccessTokens.
I've read you don't want to bring things like jwt on board, so the implementation of AccessTokens for apps is left for users.