Page MenuHomePhabricator

Ensure that SonarQube is commenting on gerrit code reviews of the Search Platform team
Open, Needs TriagePublic3 Estimated Story Points

Description

As a developer, I want timely feedback on the quality of my code, so that I can correct it before merging when needed.

We used to get inline comments from SonarQube in gerrit code reviews. This hasn't been the case lately. This can be either because our code has become good enough and has no violation (unlikely), or because of changes in the integration between SonarCloud and Gerrit.

AC:

  • SonarQube violations are reported as inline comments in CRs

Event Timeline

Gehel created this task.Oct 7 2020, 12:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 7 2020, 12:36 PM
Gehel updated the task description. (Show Details)Oct 7 2020, 12:42 PM

We disabled enough rules that it just doesn't happen as much. Inline commenting is still working though, I can dig up an example later.

Gehel added a comment.Oct 7 2020, 12:55 PM

We disabled enough rules that it just doesn't happen as much. Inline commenting is still working though, I can dig up an example later.

This is mainly on Java projects, were we did not disable any rules on those. But yes, this might be a false positive, we should make sure that comment are still getting through.

Does/Should this ticket include sonarqube for our python and scala projects?

EBernhardson set the point value for this task to 3.Oct 19 2020, 5:07 PM
Mstyles claimed this task.Oct 27 2020, 2:49 PM

@EBernhardson I think the commenting for python/scala would be included as a part of T264877

I've looked into this and unless we want to do a check on rules, then everything is working as expected as pointed out by @kostajh . However, as our master branch is failing on the quality gate, it seems that we need change something about the way we are running Sonar. Perhaps some of the rules that have been disabled should be re-enabled?

Looking at the quality gate failure, it looks like a real bug that should have been reported. This was introduced here, which was analyzed by SonarCloud without reporting any issue on November 6, and was merged shortly after. It looks to me that there is a real issue here.

Change 642203 had a related patch set uploaded (by Mstyles; owner: Mstyles):
[wikidata/query/rdf@master] [DNM] testing sonar comments

https://gerrit.wikimedia.org/r/642203

Sonar should definitely comment on this patch: https://gerrit.wikimedia.org/r/c/wikidata/query/rdf/+/642203 but the quality gate passed. There's definitely an issue, will be investigating further

Sonar should definitely comment on this patch: https://gerrit.wikimedia.org/r/c/wikidata/query/rdf/+/642203 but the quality gate passed. There's definitely an issue, will be investigating further

What are you expecting for a comment? If the quality gate fails (e.g. issues are detected) then those should get surfaced as comments.

Gehel added a comment.EditedNov 20 2020, 9:45 AM

What are you expecting for a comment? If the quality gate fails (e.g. issues are detected) then those should get surfaced as comments.

The patch above introduces a constant comparison (options.consumerGroup().equals(options.consumerGroup()) is always true). In a previous patch (since then fixed, so the warning disappeared from Sonar) it wasn't commented on in the CR, but did make the quality gate fail on master after the merge.

It might be that breaking this rule isn't enough to fail the quality gate on its own. I'm not sure if we publish sonar comments only on quality gate failure, or in all cases (that might be something to validate).

Note that running SonarLint locally (from within InitelliJ) does flag this error (rule = java:S1764). Strangely, the analysis on SonarCloud does not report any violation. Even worse, the analysis shows no code being analyzed in this CR. Maybe we did transition to Java 11 for Sonar analysis post merge, but not for branch analysis?

hashar added a subscriber: hashar.Dec 15 2020, 4:58 PM

We paired about it with @Mstyles and @Gehel there are a few different issues. One was with the java-codehealth-patch mentioning it lacked a default fallback branch. We tried to reproduce locally using:

  • Run install with Java 8: JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk-amd64 mvn clean install
  • Generate a token from https://sonarcloud.io/account/security/
  • Run analysis with Java 11: JAVA_HOME=/usr/lib/jvm/java-1.11.0-openjdk-amd64 mvn -Dsonar.login=<TOKEN HERE> sonar:sonar -X

And eventually it generated and posted the report:

[DEBUG] 18:05:38.586 Upload report
[DEBUG] 18:05:42.141 POST 200 https://sonarcloud.io/api/ce/submit?organization=wmftest&projectKey=org.wikidata.query.rdf%3Aflink-fs-swift&projectName=Flink%20Swift%20FS%20plugin%20-%20TempAuth | time=3554ms
[INFO] 18:05:42.147 Analysis report uploaded in 3561ms
[DEBUG] 18:05:42.204 GET 200 https://sonarcloud.io/api/server/version | time=56ms
[DEBUG] 18:05:42.206 Report metadata written to wikidata/query/flink-swift-plugin/target/sonar/report-task.txt
[INFO] 18:05:42.207 ANALYSIS SUCCESSFUL, you can find the results at: https://sonarcloud.io/dashboard?id=org.wikidata.query.rdf%3Aflink-fs-swift
[INFO] 18:05:42.207 Note that you will be able to access the updated dashboard once the server has processed the submitted analysis report
[INFO] 18:05:42.207 More about the report processing at https://sonarcloud.io/api/ce/task?id=AXZnXWLB798rZegomEq1
[DEBUG] 18:05:42.210 Post-jobs : 
[INFO] 18:05:42.213 Analysis total time: 26.808 s
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

So it is at https://sonarcloud.io/dashboard?id=org.wikidata.query.rdf%3Aflink-fs-swift

And the Flink Swift FS plugin - TempAuth project can now be found when doing a search on sonarcloud.io when it previously cant. So potentially the java-codehealth-patch which runs when a change is proposed is fixed?

The other issue we have is that java-codehealth-patch and the site-publish jobs run using java8. We should run mvn install with java 8 and mvn sonar:sonar with java 11. That needs a bit of refactoring in the JJB definition, probably in creating a new job template just for that purpose.

Change 649709 had a related patch set uploaded (by Hashar; owner: Hashar):
[wikidata/query/flink-swift-plugin@master] (DO NOT SUBMIT) Test change to trigger CI

https://gerrit.wikimedia.org/r/649709

Change 649709 abandoned by Hashar:
[wikidata/query/flink-swift-plugin@master] (DO NOT SUBMIT) Test change to trigger CI

Reason:
It is no more failing due to lack of a fallback branch. I guess we need to run sonar:sonar locally first in order for the project 'master' branch to be created before being able to add more analysis.

https://gerrit.wikimedia.org/r/649709

Change 649709 abandoned by Hashar:
[wikidata/query/flink-swift-plugin@master] (DO NOT SUBMIT) Test change to trigger CI

Reason:
It is no more failing due to lack of a fallback branch. I guess we need to run sonar:sonar locally first in order for the project 'master' branch to be created before being able to add more analysis.

https://gerrit.wikimedia.org/r/649709

@hashar yeah, sorry, you either need to analyse locally with main/master which is kind of a pain, or wait for a patch to be merged into the main branch so that it's analysed.

Gehel added a comment.Dec 18 2020, 8:02 AM

I've create a doc with some higher level description of what we want / need in terms of CI for Java projects. This might be useful to simplify some of the current implementation.

Change 650487 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] jjb: Sonar requires java11 in java8 site-publish jobs

https://gerrit.wikimedia.org/r/650487

Change 650487 merged by jenkins-bot:
[integration/config@master] jjb: Sonar requires java11 in java8 site-publish jobs

https://gerrit.wikimedia.org/r/650487

Gehel added a comment.EditedWed, Jan 6, 9:52 AM

After doing a few cleanups I noticed that we are running sonar analysis in 2 different and inconsistent ways. Via the java-codehealth-patch job during pre-merge patch analysis and via the maven-java8-docker-site-publish job after patches have been merged. The maven-java8-docker-site-publish was created a long time ago and might not include all the flags / env variable needed for properly publishing results (-Dsonar.*).

I think the simpler way to implement sonar analysis would be to remove the java-codehealth-patch job and to implement analysis as another step in maven-java8-docker, similar to what is being done in maven-java8-docker-site-publish. This would also remove the duplicate build step. I think that the java-codehealth-patch job was introduced to mimic what is done on the PHP side, where we want a separate non voting build for sonar analysis. But I might be missing some context here. @kostajh probably knows better.

Note that this CR removes the need of a wrapper script for the main Java build. I think a similar approach would make sense for the sonar build, which would also remove the need for using the sonar-scanner image and relying just on the maven-java11 image instead.

Change 642203 abandoned by Mstyles:
[wikidata/query/rdf@master] [DNM] testing sonar comments

Reason:

https://gerrit.wikimedia.org/r/642203