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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Github pull request decoration. #196

Merged
merged 51 commits into from Dec 2, 2019
Merged

Github pull request decoration. #196

merged 51 commits into from Dec 2, 2019

Conversation

@mwz
Copy link
Owner

mwz commented Apr 21, 2019

Since the sonar-github-plugin has been deprecated since SonarQube 7.2 the only way to get PR decoration functionality for private projects is to pay either for a SonarCloud or a Developer Edition subscription. I'm only going to say that I don't support this decision and I don't consider this to be a premium feature. Especially since it was already available for free and SonarSource made it a paid-only feature. I'll politely refrain from making any further comments about this and I'll leave it to you to judge it. 馃槃

This PR intends to bring similar functionality to sonar-scala for free for public and private projects using the Community Edition. 馃帀

It works very similarly to the old sonar-github-plugin, in a sense that it creates comments with issues introduced in the pull request. It is configurable via sonar.scala.pullrequest.* settings:

  • sonar.scala.pullrequest.provider=github;
  • sonar.scala.pullrequest.key - pull request number;
  • sonar.scala.pullrequest.github.repository - org/project, e.g. mwz/sonar-scala;
  • sonar.scala.pullrequest.github.oauth - github oauth token;
  • sonar.scala.pullrequest.issues.disable - disable posting issues;

Under the hood, it works by storing all of the identified issues in a global collection (stored in memory), which is updated by each sensor. At the end of the analysis, it talks to Github via the API to determine which issues were introduced as a part of the analysed pull request and creates inline comments for each issue.

I worked on this, with a lot of breaks, for a little while now and it's probably at a stage when I can open a draft PR to get some early feedback. It is still a WIP, with a bunch of TODOs and some things to refactor (and missing unit tests), but I think that in theory, it should work in its current state. I'm going to try to test this e2e soon before I do any more work on it.

This is should close #133 once it's ready.

Copy link
Contributor

BalmungSan left a comment

Amazing!!!, thanks for this feature, I really missed it 馃憤

Just one other thing, there are a lot of very small files in the pr package, maybe it would be good to mix them in fewer files? for some of them, just the lincese header is almost half of the file. 馃ぃ

build.sbt Outdated
"io.circe" %% "circe-generic" % circe,
"io.circe" %% "circe-generic-extras" % circe,
"org.http4s" %% "http4s-blaze-client" % http4s,
"org.http4s" %% "http4s-circe" % http4s,

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 21, 2019

Contributor

Yeih, Typelevel friends 馃樃

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 24, 2019

Contributor

http4s v0.20.0 was released a couple of days ago :)

val defaultPrinter: Printer = Printer.noSpaces.copy(dropNullValues = true)
implicit val config: Configuration = Configuration.default.withSnakeCaseMemberNames

implicit def jsonEncoderOf[F[_]: Applicative, A](implicit encoder: Encoder[A]): EntityEncoder[F, A] =

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 21, 2019

Contributor

I love context bounds, specially when one does not need the implicit itself, but they are just needed as transitive dependencies (which this is the case).
However, I really do not like to mix them with implicit parameters, sometimes that causes problems. See this as a reference.

What about just using bounds in this case?

 implicit def jsonEncoderOf[F[_]: Applicative, A: Encoder]: EntityEncoder[F, A] =

(PS: I just checked the source code of http4s and noticed they had the same way as you. But, apparently they do that because the need the encoder explicitly, which is not our case here).

This comment has been minimized.

Copy link
@mwz

mwz Apr 21, 2019

Author Owner

good point, I'll change it

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 21, 2019

Contributor

Oh, I just realized that since I deleted the comment by mistake and restore it from a copy paste, I lost formatting and the link in "see this". Sorry for that.
I just re-added the link if you are interested.

This comment has been minimized.

Copy link
@mwz

mwz Apr 21, 2019

Author Owner

cool, thanks

@ScannerSide
@InstantiationStrategy(InstantiationStrategy.PER_BATCH)
final class GlobalIssues {
// As far as I know this won't be accessed concurrently.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 21, 2019

Contributor

I am not 100% sure that is a fact.
It is true that the analysis seems serial, but that is outside of our control.
Maybe, someday, sonar decides to run the analysis on parallel. Also, since this is, probably, responsibility of the scanner. Maybe there are custom scanners out there that already do that, who knows.

Since this is outside of our domain, IMHO is better to play safe.
Either, use synchronization or (even better?) a Java ConcurrentHashMap.

This comment has been minimized.

Copy link
@mwz

mwz Apr 21, 2019

Author Owner

That's a fair point, I'll use Scala's concurrent TrieMap instead.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 21, 2019

Contributor

I have the impression, I had read somewhere that the Scala's concurrent collections were totally broken, and that it was better to use the Java ones.
However, since most of the time I had immutable collections, I really never need that. And I can't find any proper source for supporting that statement.
I think it would be good to investigate about that.

This comment has been minimized.

Copy link
@mwz

mwz Apr 21, 2019

Author Owner

There used to be some issues with the TrieMap in the past, but I think they fixed those. Anyway, I couldn't find anything as nice as the merge function from the ConcurrentHashMap, so I went with that.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Apr 21, 2019

Just one other thing, there are a lot of very small files in the pr package, maybe it would be good to mix them in fewer files? for some of them, just the lincese header is almost half of the file. 馃ぃ

yeah no worries, I'll try to compress the code into fewer files

mwz added 3 commits Apr 22, 2019
@@ -147,7 +147,7 @@ object GithubPrReviewJob {
// TODO: This is quite grim.
def allCommentsForIssues(
issues: Map[InputFile, List[Issue]],
mappedPatches: Map[String, ErrorOr[Map[FileLine, PatchLine]]],

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Apr 22, 2019

Contributor

Maybe the type alias is not longer required?

This comment has been minimized.

Copy link
@mwz

mwz Apr 22, 2019

Author Owner

Maybe, but I haven't done much error handling yet so I'll hold off with removing it until later if it turns out it's not used anywhere.

mwz added 2 commits Apr 22, 2019
@jorkzijlstra

This comment has been minimized.

Copy link

jorkzijlstra commented May 2, 2019

Thanks for working on this project. We have been using it for some time already

We already went for the SQ (v7.7) developer license and did some testing with the pr decoration.

Adding the next config will enable pr decoration:

"sonar.pullrequest.github.repository" -> "organistion/repo",
"sonar.pullrequest.branch" -> "brach",
"sonar.pullrequest.base" -> "master",
"sonar.pullrequest.key" -> "1"

The main issue that we have is that we have to commit sonar.pullrequest.key to the code. This is not something you want to do for all of you branches / Pr's, but also haven't found a solution yet. I see in your code you are also expecting this to be defined.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented May 3, 2019

Hi @jorkzijlstra, thanks for your feedback.

sonar.pullrequest.key isn't really the best name for this setting and I'll probably change our sonar.scala.pullrequest.key to sonar.scala.pullrequest.number because it is supposed to represent the pull request number, e.g. 196 in the case of this PR.

You shouldn't hardcode this value into your code as it's dynamic and it will change for each pull request. You'd want it to come from your CI which triggers sonnar-scanner for each pull request, e.g. if you use CircleCI you can extract if from the CIRCLE_PULL_REQUEST environment variable, Travis has TRAVIS_PULL_REQUEST and other CI solutions that integrate with Github provide similar functionality.

mwz added 7 commits May 4, 2019
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented May 6, 2019

I've been testing this here #200.

mwz added 8 commits May 6, 2019
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented May 19, 2019

Everything looks good!
I'll add unit tests and address some of the important TODOs soon.

mwz added 11 commits May 30, 2019
# Conflicts:
#	build.sbt
@mwz mwz mentioned this pull request Oct 16, 2019
mwz added 12 commits Nov 3, 2019
# Conflicts:
#	.gitignore
#	.scalafmt.conf
#	build.sbt
#	project/plugins.sbt
#	src/main/scala/com/mwz/sonar/scala/scalastyle/ScalastyleSensor.scala
#	src/main/scala/com/mwz/sonar/scala/scapegoat/ScapegoatSensor.scala
#	src/main/scala/com/mwz/sonar/scala/scoverage/ScoverageSensor.scala
#	src/test/scala/com/mwz/sonar/scala/ScalaPluginSpec.scala
#	src/test/scala/com/mwz/sonar/scala/scalastyle/ScalastyleSensorSpec.scala
#	src/test/scala/com/mwz/sonar/scala/scapegoat/ScapegoatSensorSpec.scala
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Dec 2, 2019

Hey @BalmungSan, I'm going to merge this to master and raise a few additional PRs to get this over the line. I'm not too far off in terms of getting this finished.

@mwz mwz marked this pull request as ready for review Dec 2, 2019
@mwz mwz changed the title WIP Github pull request decoration. Github pull request decoration. Dec 2, 2019
@mwz mwz merged commit e640a24 into master Dec 2, 2019
3 checks passed
3 checks passed
ci/circleci: test Your tests passed on CircleCI!
Details
security/snyk - build.sbt (mwz) No new issues
Details
security/snyk - project/build.sbt (mwz) No manifest changes detected
@mwz mwz deleted the github-pr-decoration branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.