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

Support for private GitHub Enterprise installations #379

Closed
satorg opened this issue Mar 19, 2020 · 6 comments · Fixed by #384
Closed

Support for private GitHub Enterprise installations #379

satorg opened this issue Mar 19, 2020 · 6 comments · Fixed by #384

Comments

@satorg
Copy link
Contributor

satorg commented Mar 19, 2020

The base GitHub API URLs are hard-coded inside the GithubAPIv3Config class:

case class GithubAPIv3Config(
    baseUrl: String = "https://api.github.com/",
    authorizeUrl: String =
      "https://github.com/login/oauth/authorize?client_id=%s&redirect_uri=%s&scope=%s&state=%s",
    accessTokenUrl: String = "https://github.com/login/oauth/access_token"
)

which makes impossible to leverage github4s in those companies which use their private GitHub installations and obviously different base API URLs.

Currently it seems there's no way to override these URLs.

@satorg
Copy link
Contributor Author

satorg commented Mar 19, 2020

I believe I would be great to allow to override these values or/and read them from system properties and configuration files.

@BenFradet
Copy link
Contributor

Definitely agreed 👍

@BenFradet
Copy link
Contributor

I think we could turn that case class into an implicit parameter when instantiating Github[F](client, token)

@satorg
Copy link
Contributor Author

satorg commented Mar 19, 2020

Agree, I think it could look something like:

def apply[F[_]: ConcurrentEffect](
      accessToken: Option[String] = None,
      timeout: Option[Duration] = None
  )(implicit ec: ExecutionContext, config = GithubAPIv3Config.default()): Github[F]

where GithubAPIv3Config.default() creates config for the public GitHub (https://api.github.com/ etc)
although my concern here is that GithubAPIv3Config looks more like implementation details.
WDYT?

@BenFradet
Copy link
Contributor

yes I think it needs renaming, maybe something like:

final case class GithubConfig(
  baseUrl: String = "https://api.github.com/",
  authorizeUrl: String = "https://github.com/login/oauth/authorize?client_id=%s&redirect_uri=%s&scope=%s&state=%s",
  accessTokenUrl: String = "https://github.com/login/oauth/access_token"
)

object GithubConfig {  
  implicit val default: GithubConfig = GithubConfig()
}

and then have:

class Github[F[_]: Sync](
    client: Client[F],
    accessToken: Option[String]
)(implicit ghConfig: GithubConfig)

👍

@satorg
Copy link
Contributor Author

satorg commented Mar 22, 2020

@BenFradet, I submitted a PR #384, could you take a look please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants