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:
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.
@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
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.
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?
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:
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
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.
@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.
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
Mentioned in SAL (#wikimedia-releng) [2020-12-18T14:52:01Z] <hashar> Updating https://integration.wikimedia.org/ci/job/wikidata-query-flink-swift-plugin-maven-java8-docker-site-publish/ to run sonar with Java 11 (build remains on Java 8) T264873
Change 650487 merged by jenkins-bot:
[integration/config@master] jjb: Sonar requires java11 in java8 site-publish jobs
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:
Change 666869 had a related patch set uploaded (by Gehel; owner: Gehel):
[integration/config@master] Replace the java-codehealth-patch by adding step in the standard job
Change 667113 had a related patch set uploaded (by Gehel; owner: Gehel):
[mediawiki/tools/mwdumper@master] Minimal configuration of Sonar maven plugin.
Change 667114 had a related patch set uploaded (by Gehel; owner: Gehel):
[analytics/refinery/source@master] Minimal configuration of Sonar maven plugin.
Change 667115 had a related patch set uploaded (by Gehel; owner: Gehel):
[analytics/wmde/toolkit-analyzer@master] Minimal configuration of Sonar maven plugin.
Change 667116 had a related patch set uploaded (by Gehel; owner: Gehel):
[wikimedia/discovery/discovery-maven-tool-configs@master] Minimal configuration of Sonar maven plugin.
Change 667118 had a related patch set uploaded (by Gehel; owner: Gehel):
[search/xgboost@master] Minimal configuration of Sonar maven plugin.
Change 667116 merged by jenkins-bot:
[wikimedia/discovery/discovery-maven-tool-configs@master] Minimal configuration of Sonar maven plugin.
Change 667115 merged by jenkins-bot:
[analytics/wmde/toolkit-analyzer@master] Minimal configuration of Sonar maven plugin.
Change 667113 merged by jenkins-bot:
[mediawiki/tools/mwdumper@master] Minimal configuration of Sonar maven plugin.
Change 667114 merged by Ottomata:
[analytics/refinery/source@master] Minimal configuration of Sonar maven plugin.
Change 667118 merged by Gehel:
[search/xgboost@master] Minimal configuration of Sonar maven plugin.
Change 667984 had a related patch set uploaded (by Gehel; owner: Gehel):
[integration/gearman-java@master] Add minimal sonar configuration.
Change 666869 merged by jenkins-bot:
[integration/config@master] Replace the java-codehealth-patch by adding step in the standard job
Mentioned in SAL (#wikimedia-releng) [2021-03-03T10:33:48Z] <hashar> REplaced java-codehealth-patch job in favor of running sonar:sonar inline in all the java jobs. Thanks gehel! # T264873 | https://gerrit.wikimedia.org/r/c/integration/config/+/666869/
Change 667984 merged by jenkins-bot:
[integration/gearman-java@master] Add minimal sonar configuration.
Change 669760 had a related patch set uploaded (by Gehel; owner: Gehel):
[integration/config@master] Run sonar analysis with proper parameters for Java projects.
Change 669760 merged by jenkins-bot:
[integration/config@master] Run sonar analysis with proper parameters for Java projects.
After some help from SonarSource I now have a better understanding. With the way Zuul merges our patches before the build, Sonar sees the project being already on the HEAD of the target branch and does not see any diff. I'll work with @hashar to find another way of checking out projects.
Note that this analysis was done as part of T264877 and applies to that ticket as well.
Looking at the Sonar sources, detection of the starting point of the branch is doing by searching for refs in multiple places, first locally, then on remote. In our case, the local ref isn't the correct one.
I'm still working with @hashar on finding a long term solution, but we seem to be close!
Change 673517 had a related patch set uploaded (by Gehel; owner: Gehel):
[integration/config@master] jjb: fix sonar code comparison
Change 673517 merged by jenkins-bot:
[integration/config@master] jjb: fix sonar code comparison
Change 673981 had a related patch set uploaded (by Gehel; owner: Gehel):
[integration/config@master] WIP
Change 673981 merged by jenkins-bot:
[integration/config@master] maven-java8-docker: fix sonar analysis
Looks like there are still cases where no changes are reported (see https://gerrit.wikimedia.org/r/c/wikidata/query/rdf/+/665082). Moving back to in progress.
Change 674272 had a related patch set uploaded (by Gehel; owner: Gehel):
[integration/config@master] Add debug options to sonar analysis.
Change 674272 merged by jenkins-bot:
[integration/config@master] Add debug options to sonar analysis.
Change 674277 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] jjb: git topology debug for Maven Sonar analysis
Change 674277 abandoned by Hashar:
[integration/config@master] jjb: git topology debug for Maven Sonar analysis
Reason:
That added a few git commands to inspect the repository state, so essentially for debugging purposes. We can restore it later on if we really find them useful.
Sonar analysis is enabled for all Search Platform projects. There is still an issue of the differential analysis not being done correctly on CR that result in a merge commit, but this is low enough priority that it makes more sense to wait for the migration to Gitlab.