Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

Support PR decoration for github enterprise #434

Merged
merged 8 commits into from Jun 11, 2020

Conversation

jinwoo-k
Copy link
Contributor

No description provided.

Copy link
Contributor

@BalmungSan BalmungSan left a comment

Choose a reason for hiding this comment

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

Looks good, but I has been I while since the last time I contributed here. So I may be overseeing something.

PS: You can add this as the description of your PR to automatically close the issue when merged.

Fixes #414


As an additional note, is there a way to (unit) test this?

@@ -93,6 +93,7 @@ final class GlobalConfig(config: Configuration) {
config.getAs[String](PR_NUMBER),
ConfigError(s"Please provide a pull request number ($PR_NUMBER).")
)
githubApiUrl = config.getAs[String](PR_GITHUB_API_URL).getOrElse(DEFAULT_GITHUB_API_URL)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you may get this value directly as an URI, instead of as an String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BalmungSan
Thanks for your comments.
I think it seems to be to create a getAs [T <: Uri] method in SonarConfig.
However, if i do, many existing codes (ex. Val prUri: String> val val prUri: Uri, etc.) are likely to be affected. Is this an okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right I got confused and for a moment I thought we were using pureconfig which does has that method.

However, you may want to give a look to how val baseUrl is parsed.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I agree that the url should be parsed otherwise it could easily blow up later when you're calling Uri.unsafeFromString, as the name suggests :)

You could do something like that:

githubApiUrl <- EitherT.fromEither(
  config
    .getAs[String](PR_GITHUB_API_URL)
    .map(Uri.fromString)
    .fold[ConfigErrorOr[Uri]](Right(DEFAULT_GITHUB_API_URL))(_.leftMap(f => ConfigError(f.sanitized)))
)

This is slightly different from the baseUrl, because we can accept a missing value (and default to DEFAULT_GITHUB_API_URL), but we want to fail early when there's a parsing error.

You'll also need to change the DEFAULT_GITHUB_API_URL to Uri.uri("https://api.github.com") and you should be on the safe side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified it. Please make sure that it's the right one.

@@ -40,24 +40,25 @@ object Github {
def apply[F[_]: Sync](client: Client[F], pr: GlobalConfig.PullRequest): Github[F] =
new Github[F] {
val auth: Header = Header("Authorization", s"token ${pr.github.oauth}")
val userUri: String = s"${pr.github.apiurl}/user"
Copy link
Member

Choose a reason for hiding this comment

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

Once you have an Uri here, you'll need to use the uri syntax, e.g. pr.github.apiurl / "user".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified it. Please make sure that it's the right one.

@mwz
Copy link
Member

mwz commented Apr 27, 2020

Thanks for contributing! I don't have access to github enterprise to test it, so if that's ok with you I'm going to rely on you to check against a local snapshot if it works as expected 🙏

@jinwoo-k
Copy link
Contributor Author

jinwoo-k commented Jun 3, 2020

The modification was delayed due to personal reasons.
It operates on the github entity as follows:

스크린샷 2020-06-03 오후 2 34 36

@jinwoo-k jinwoo-k requested a review from mwz June 3, 2020 05:40
@mwz
Copy link
Member

mwz commented Jun 4, 2020

awesome, thanks @jinwoo-k I'll take a look soon

@@ -158,6 +158,10 @@ class GithubSpec extends AnyFlatSpec with Matchers with ScalaCheckDrivenProperty
val http = AuthedRoutes.of[String, IO] {
case POST -> Root / "repos" / "owner" / "repo" / "statuses" / _ as _ =>
Ok(response)
case a => {
Copy link
Member

@mwz mwz Jun 10, 2020

Choose a reason for hiding this comment

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

Looks good, thanks for making the changes - I'd only remove this case and I'm happy to merge 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! The unnecessary codes created during the test were committed. They were removed.
Thank you!

@mwz
Copy link
Member

mwz commented Jun 11, 2020

Great, thanks for your contribution. I'll try to release it in a day or so.

@mwz mwz changed the title Support PR decoration for github enterprise (#414) Support PR decoration for github enterprise Jun 11, 2020
@mwz mwz merged commit 379330d into sonar-scala:master Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants