Skip to content
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

Merged
merged 13 commits into from
Nov 17, 2020

Conversation

reimai
Copy link
Contributor

@reimai reimai commented Nov 5, 2020

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:

  • move accessToken into the http client
  • make it a monad

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.

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #567 (15cb1a8) into master (965fba3) will decrease coverage by 17.29%.
The diff coverage is 37.77%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
github4s/src/main/scala/github4s/Github.scala 0.00% <0.00%> (-100.00%) ⬇️
...ub4s/src/main/scala/github4s/http/HttpClient.scala 9.75% <0.00%> (-87.22%) ⬇️
.../github4s/interpreters/ActivitiesInterpreter.scala 100.00% <ø> (ø)
...scala/github4s/interpreters/GistsInterpreter.scala 100.00% <ø> (ø)
...thub4s/interpreters/OrganizationsInterpreter.scala 100.00% <ø> (ø)
...la/github4s/interpreters/ProjectsInterpreter.scala 100.00% <ø> (ø)
...cala/github4s/interpreters/StaticAccessToken.scala 0.00% <0.00%> (ø)
...s/src/main/scala/github4s/modules/GithubAPIs.scala 0.00% <0.00%> (-100.00%) ⬇️
...ala/github4s/interpreters/GitDataInterpreter.scala 100.00% <100.00%> (ø)
...cala/github4s/interpreters/IssuesInterpreter.scala 100.00% <100.00%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 965fba3...c3a44ec. Read the comment docs.

@reimai reimai marked this pull request as draft November 6, 2020 08:17
making it a function so that GithubApp implementation could catch an expired token exception
@BenFradet BenFradet self-requested a review November 9, 2020 15:59
Copy link
Contributor

@BenFradet BenFradet left a 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[_]] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trait AccessTokens[F[_]] {
trait AccessToken[F[_]] {

def withAccessToken[T](f: Option[String] => F[GHResponse[T]]): F[GHResponse[T]]
}

object AccessTokens {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
object AccessTokens {}

/**
* A simple static version
*/
class StaticAccessTokens[F[_]](accessToken: Option[String]) extends AccessTokens[F] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class StaticAccessTokens[F[_]](accessToken: Option[String]) extends AccessTokens[F] {
class StaticAccessToken[F[_]](accessToken: Option[String]) extends AccessToken[F] {

@reimai
Copy link
Contributor Author

reimai commented Nov 10, 2020

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.

@reimai reimai requested a review from BenFradet November 12, 2020 08:04
@reimai reimai marked this pull request as ready for review November 16, 2020 09:47

object GithubAPIv3 {

def noAuth[F[_]: Sync](client: Client[F], config: GithubConfig) =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing return type

microsite/docs/docs.md Outdated Show resolved Hide resolved
microsite/docs/docs.md Outdated Show resolved Hide resolved
@reimai reimai requested a review from BenFradet November 17, 2020 11:56
Copy link
Contributor

@BenFradet BenFradet left a 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 👍

Copy link
Member

@juanpedromoreno juanpedromoreno left a 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!

@juanpedromoreno juanpedromoreno merged commit df21046 into 47degrees:master Nov 17, 2020
@juanpedromoreno juanpedromoreno added the breaking-change A breaking change that needs to be treated with consideration label Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A breaking change that needs to be treated with consideration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants