[FOLIO-1763] Sonarcloud analysis not updating GitHub PR and Checks Created: 31/Jan/19 Updated: 03/Jun/20 Resolved: 28/Feb/19 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Bug | Priority: | P2 |
| Reporter: | John Malconian | Assignee: | David Crossley |
| Resolution: | Done | Votes: | 0 |
| Labels: | ci, platform-backlog | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||||||||||
| Sprint: | Core: Platform - Sprint 56, Core: Platform - Sprint 58, Core: Platform - Sprint 57 | ||||||||||||||||
| Story Points: | 5 | ||||||||||||||||
| Development Team: | Core: Platform | ||||||||||||||||
| Description |
|
I've received and confirmed multiple cases in which results of Sonarqube analysis are not updated on Github PRs even though the analysis is successfully run and processed by Sonarcloud. Symptoms of this include a Sonarcloud status of "queued" on Github. The issue appears to be intermittent and may indicate a bug on the Sonarcloud end. The Maven-based scans do update the Checks tab. However their scans seem erratic. |
| Comments |
| Comment by John Malconian [ 04/Feb/19 ] |
|
I notice that this ui-requests PR doesn't invoke the Sonarqube scan - https://jenkins-aws.indexdata.com/job/folio-org/job/ui-requests/job/PR-234/2/ - So that's an obvious problem to investigate first, I think. |
| Comment by John Malconian [ 04/Feb/19 ] |
|
I see the following warning in Sonarcloud for branches of UI projects. Not sure what is going here. Could not find ref 'master' in refs/heads or refs/remotes/origin. You may see unexpected issues and changes. Please make sure to fetch this ref before pull request analysis. |
| Comment by John Malconian [ 04/Feb/19 ] |
|
This seems to resemble some of the issue we are seeing: https://community.sonarsource.com/t/pr-decoration-for-github-no-longer-working/5875/7 |
| Comment by David Crossley [ 05/Feb/19 ] |
|
Regarding the part above about "ui-requests PR doesn't invoke the Sonarqube scan". |
| Comment by John Malconian [ 05/Feb/19 ] |
|
That sounds about right. Unlike the Maven modules, Sonarqube analysis is an optional config parameter for Stripes/UI modules that is false by default (although it probably should be true by default). Aside from the ui-requests issue, however, I think that I am seeing that no Sonarqube analysis is actually processed on Sonarcloud for any of the UI/Stripes modules on non-master branches and pull requests. For example: https://sonarcloud.io/project/issues?branch=UICHKIN-75&id=org.folio%3Aui-checkin&resolved=false Sonarqube reports "no issues", but I don't think any thing is actually processed. Also, for Maven modules, Sonarcloud processing seems hit or miss. The problems started to occur two-three weeks ago after Sonarcloud made some significant changes to how results are analyzed and how Github PRs are decorated. There may be some hints at the end of this thread - https://community.sonarsource.com/t/pr-decoration-for-github-no-longer-working/5875/7 - but we may need to post an inquiry to the community board. |
| Comment by David Crossley [ 05/Feb/19 ] |
|
Here is another useful report, having similar symptoms, and active today: |
| Comment by David Crossley [ 05/Feb/19 ] |
|
So how are the Sonar scans run for those other ui-* modules that are not configured in their Jenkinsfile. |
| Comment by David Crossley [ 05/Feb/19 ] |
|
I confirm that processing seems to be not happening for non-master branches and PRs. Looking at ui-circulation today, there are 17 issues reported for a master branch build, but none for the recent PR and its branch. |
| Comment by David Crossley [ 05/Feb/19 ] |
|
The linked community posts mention ensuring doing 'git fetch' before running sonar-scanner. We use the Jenkins "checkout" function, so i presume that it is doing that. Investigated our Jenkins "Checkout" stage to try to find any difference between the Maven-based builds and the NPM-based builds. The vars/buildNPM.groovy has an extra section "RelativeTargetDirectory" added a couple of months ago, but only merged 9 days ago with jenkins-pipeline-libs/pull/57 (
I am not clear what that does, but wonder if that could that be the cause? |
| Comment by David Crossley [ 07/Feb/19 ] |
|
Here is our post today to the SonarSource Community forum: |
| Comment by David Crossley [ 08/Feb/19 ] |
|
We indicated above that sonar is not running on non-master branches and PRs. That seems to be not true. See this one today where i added deliberate new "code smell": So i adusted this issue Summary, and the post title: He is still puzzled about why the Sonar result is not reported back to GitHub Checks for the NPM-based builds. He asked if it is happening consistently. I said yes. If anyone sees any differently then please show an example branch/PR. |
| Comment by John Malconian [ 15/Feb/19 ] |
|
David Crossley I believe I've made some significant progress on this issue. Github PR decoration for NPM-based builds There is a command ('npm version') in the NPM build pipeline that is executed prior to Sonarqube analysis that makes a git commit on the Jenkins checkout during the build. This commit updates the SHA1 of HEAD so that it no longer matches the original PR SHA1 on Github and, therefore, confuses Sonarqube. I've added the option '--no-git-tag-version' to the command in the shared pipeline libraries so that no git commit is made during the build. The result is that SonarCloud GitHub PR decoration now works on NPM-based PRs. The Sonarcloud/Sonarqube Scanner warning "Could not find ref 'master' in refs/heads or refs/remotes/origin. You may see unexpected issues and changes. Please make sure to fetch this ref before pull request analysis" that occurs during branch builds on all projects. This warning totally makes sense, because, unlike PR builds in Jenkins, ref 'master' is indeed not fetched on branch builds - only the ref of the branch is fetched. But Sonarqube now requires ref 'master' when scanning branch builds. The solution was to add the command 'git fetch --no-tags ${env.projUrl} +refs/heads/master:refs/remotes/origin/master'
prior to invoking the Sonarqube scan during branch builds (for both Maven and NPM-based builds). After this modification, I've verified that the warning is no longer generated. I've merged these changes into jenkins-pipeline-libs in PR-58. https://github.com/folio-org/jenkins-pipeline-libs/pull/58. See the PR for exact code changes. I'm going to keep this issue open so that you can monitor the status of these changes over the next few days. It's still not entirely clear to me why some Maven-based PRs are intermittently not decorated and if that issue is related to either I've the changes I've made or something else entirely. When/If you feel the issues have been adequately resolved, then go ahead and close this issue. |
| Comment by David Crossley [ 18/Feb/19 ] |
|
That is great news. I have done various PRs today, and the Sonar checks are behaving. Will monitor further. If developers see different behaviour then please tell me. On an open PR, the Sonar check result should reported to the "Show all checks" panel of the Conversation tab, and to the actual "Checks" tab (need to refresh page to see the latter). |
| Comment by David Crossley [ 18/Feb/19 ] |
|
Updated our SonarSource post: |
| Comment by David Crossley [ 19/Feb/19 ] |
|
Drat. Assessed 38 pull requests since the jenkins-pipeline-libs/pull/58 was merged to master. 30 had the decoration of the Sonar result. 8 did not. Of those 8: Apart from those 6, the other two were: https://github.com/folio-org/ui-checkin/pull/164 (email notification for opened PR: Mon, 18 Feb 2019 08:11:53 -0800) Following through to Sonarcloud as Admin for those two repos shows no problems for "Background Tasks". |
| Comment by Aliaksei Chumakou [ 20/Feb/19 ] |
|
David Crossley it looks like SonarCloud test coverage on the new code had stopped working for ui-orders a while ago (always 0.0%). |
| Comment by David Crossley [ 21/Feb/19 ] |
|
Aliaksei Chumakou Not sure yet if that "coverage on the new code" is related to this
|
| Comment by David Crossley [ 21/Feb/19 ] |
|
Assessed the next 46 PRs for relevant repos (up to 20190220 23:43 UTC). https://github.com/folio-org/mod-circulation/pull/190 For each of those 4 their analysis was run, just not reported back to PR. |
| Comment by David Crossley [ 22/Feb/19 ] |
|
Updated our SonarSource post: The helpful Janos at SonarSource replied to my recent summaries. They are investigating at their end. It is a great benefit of truly open source that we can actually demonstrate our findings to the post. |
| Comment by David Crossley [ 25/Feb/19 ] |
|
Found some documentation about this feature change: As explained there, now the results are summarised and displayed on the Checks tab of the PR. Sonar no longer displays each detected code issue on the front Conversation tab of the PR. |
| Comment by David Crossley [ 26/Feb/19 ] |
|
In our SonarSource post linked above, Janos provided a 'curl' query to verify the decoration. Adding the pattern here for future reference: curl https://api.github.com/repos/folio-org/<repo-name>/commits/<sha1-of-last-commit-in-the-set>/check-runs \ -H 'Accept: application/vnd.github.antiope-preview+json' | jq . |
| Comment by David Crossley [ 26/Feb/19 ] |
|
We reckon that we have solved the apparent "intermittent" decoration of the PR Checks tab. After creating a branch for a PR, then the master might be subsequently updated. When our Jenkins is preparing for the sonar scan, the master is merged into the PR head. That of course can make a different SHA1 which does not correlate with the SHA1 that is being decorated with the Sonar result. All repositories will now have the branch protection setting "Require branches to be up to date before merging". |
| Comment by David Crossley [ 26/Feb/19 ] |
|
To summarise, there were three parts: 1) An extra commit was being made by our Jenkins when preparing for the scan by doing 'npm version'. Instead do 'npm --no-git-tag-version version'. 2) Remove the Warning note: Could not find ref 'master' in refs/heads or refs/remotes/origin ... by doing the following before doing the Sonar scan: git fetch --no-tags ${env.projUrl} +refs/heads/master:refs/remotes/origin/master
3) Add the branch protection setting "Require branches to be up to date before merging". |
| Comment by David Crossley [ 26/Feb/19 ] |
|
Now "In review". Will monitor the PRs for the next day. |
| Comment by David Crossley [ 28/Feb/19 ] |
|
At our SonarSource post:
Even if we sign up, cannot see that ticket. So watch the SonarSource Announcements forum, e.g. there is a beta of automatic analysis. |
| Comment by David Crossley [ 28/Feb/19 ] |
|
Assessed our next 23 relevant PRs from yesterday, since
Some others were still open PRs which did not have the Checks tab with Sonar report. This was because they had not yet done the up-to-date branch requirement. All is now well, so closed this ticket. |