SonarCloud Code Analysis Expected- onoging issues

Hi @Alexandre_Holzhey,

thanks and sorry for the late response.
i did saw this warning now on the PR that are not decorated.

i’m not sure though i understand your suggestion when this happens.
when we call the sonarqube we do it with the following args.
$(echo sonarqube -Dsonar.pullrequest.branch=${GITHUB_HEAD_REF} -Dsonar.pullrequest.key=${PR_KEY} -Dsonar.pullrequest.base=${GITHUB_BASE_REF})

as mentioned previosely wether i use the merge commit for the checkout or specficially ask to checkout the head of the commit (which i assume its the GITHUB_SHA you’ve mentioned), the Sonar always tries to decorate the merged commit.

so i should force sonarqube to use the github_sha and not the merge commit while adding the following:
-Dsonar.scm.revision=${{ github.event.pull_request.head.sha }} ?

thanks,
Chen

just wanted to update that forcing SonarCloud to decorate the github_sha using -Dsonar.scm.revision=${{ github.event.pull_request.head.sha }} looks working.
even when using the checkout@v2 with the default merge commit.

will have to monitor it of course for multiple repos and multiple PRs to see if the issue happens again.
Chen

Thanks @clevi for the updates, indeed using this should be a good workaround:

-Dsonar.scm.revision=${{ github.event.pull_request.head.sha }}

Note that this only forces what is decorated, it doesn’t change what is analyzed. The scanner analyzes whatever it finds in its working tree. And with checkout@v2 so far it seems that’s always the merge commit. (I’m still investigating.)
When the base branch has moved significantly compared to the PR, line numbers in the decoration may no longer match the actual code. However, I would expect that’s not the typical case, and this workaround of forcing the SHA1 to decorate will work well most of the time. In the rare cases when the base has moved too much, you can rebase the PR on its base to get better results.

Our goal for the scanner is to do the most useful thing without additional configuration. And that is analyzing and decorating the HEAD of the PR. If it’s not possible to enforce the working tree to be the HEAD of the PR, I’m inclined to force -Dsonar.scm.revision=${{ github.event.pull_request.head.sha }} by default if possible, even if the result may sometimes be wrong, as explained above.

HI @janos,
as mentioned it is possible to checkuot the HEAD of the PR using checkout@v2 and not only the merge commit.

it can be done using:

  • uses: actions/checkout@v2 with: ref: ${{ github.event.pull_request.head.sha }}

but, the problem was although it might be the analyzed PR, we need to force the sonar to also decorate it and not the merge commit.
is that make sense?

Thanks @clevi,

I see, so currently the most sane option has two elements:

  • Ensure the working tree is the HEAD of the PR, with uses: actions/checkout@v2 with: ref: ${{ github.event.pull_request.head.sha }}
  • Ensure the decoration will target the HEAD of the PR, with -Dsonar.scm.revision=${{ github.event.pull_request.head.sha }}

Thanks for the clarification!

We’re running sonarcloud in a github action but are having something that sounds like the same issue. I’m unsure how to get something like -Dsonar.scm.revision=${{ github.event.pull_request.head.sha }} added to the action. What would I change in the action code below that would implement this work around.

    - name: SonarCloud Scan
      uses: sonarsource/sonarcloud-github-action@master
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}

@dlwire I see you are using sonarsource/sonarcloud-github-action, and indeed it was not possible to pass extra parameters as in the workaround. We merged just now an improvement to the action to make this possible, so now you can do like this:

- name: SonarCloud Scan
  uses: sonarsource/sonarcloud-github-action@master
  with:
    args: >
      -Dsonar.scm.revision=${{ github.event.pull_request.head.sha }}
  env:
    GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
    SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
1 Like

TL;DR

We’re working on an improvement to use ${{ github.event.pull_request.head.sha }} by default as the sha1 to decorate. This should lead to more stable experience. I will post an update here when it’s rolled out.

On the other hand, ensuring the working tree is the HEAD of the PR with uses: actions/checkout@v2 with: ref: ${{ github.event.pull_request.head.sha }} is out of our control. It’s recommended for consistent results, but our experience shows no problems in practice even without that.

Longer version

We’ve made some progress understanding the issue. Currently we use the sha1 in GITHUB_SHA to decorate as a baseline. Then, we detect if that value is the special merge commit automatically generated by GitHub, in which case we fall back to using the head of the PR. This seems to work most of the time, in fact I’m not able to fabricate an example where it doesn’t. But as all the posters in this thread can attest, this clearly does not always work.

We used to believe that GITHUB_SHA is always the head of the PR, but according to the docs, that’s not the case when the trigger event is pull_request. In this case, the value is “the last merge commit” of the branch, which happens to be a merge commit automatically generated by GitHub.

By making ${{ github.event.pull_request.head.sha }} the new default, we expect a more stable behavior.

The workaround to use ${{ github.event.pull_request.head.sha }} by default as the sha1 to decorate is now live. Please let us know in this thread if you still experience the missing Check.

Hi @janos,

I tried it on couple of days ago and it seems that we didn’t see any missing checks since then. I’m going to keep an eye on it and let you know if the issue reappears :slight_smile:

Hi

i’m reviewing all repos and make the needed changes after your recent fixes from Sonar side.
as discussed and agreed i’m checking out the specific ref ( actions/checkout@v2 with: ref: ${{ github.event.pull_request.head.sha }}).
now on the sonarqube command i’m not providing the scm.revision paraemter, since you’ve mentioned its now using the head.sha by default.

Sonar analyses the PR properly, and it also decorated on the PR.
BUT, the status check is still on pending state.


backgroundTaskID: AXRjUVhSY-3lxofoFyjF

have i missed anything?

thanks
Chen

1 Like

The same happens now with our repository. Everything works except the pending state.

1 Like

We are facing the same issue here in one of our repositories (private), the comment is added in PRs but not the check.

We are also having the same issue, but using Azure DevOps Pipelines. One of our private repos is failing to notify GitHub for the status check. Another private repo is functioning as expected. In both cases the Sonar analysis is performed, Sonar bot comments to the Pull Request accordingly, but one of the two fails to set the status check. Everything was functioning normally for our pull request build performed at 1:14 PM UTC on Wednesday September 2nd, but every pull request since has failed to notify. For both of our repos the SHA of the merge commit is checked out.

Hi @Michael_Staszewski,
i think in your case the problem is the new changes that the default SHA that being analyzed is the head of the PR.

as far as i understand it you should update your checkout phase to checkout the head of the PR and not the merged commit.

    actions/checkout@v2 
      with: 
        ref: ${{ github.event.pull_request.head.sha }}

Hi @janos

any news regarding the current issue?
are you familiar with any current regression?
we have many PRS on different repos that are stuck.

thanks!

We are experiencing this on all of repos. We see the comment and the checks are being run and passing, but we are not getting the status check. This has just happened recently, and is currently blocking our PRs.

I am using Azure DevOps Pipelines, not GitHub Actions. AzDO does have a checkout task, but I see no way to control which SHA is used. The built-in AzDO vars provide a single variable (Build.SourceVersion) for grabbing the commit ID and it points to whatever SHA was used to trigger the pipeline run; the merge commit in my case. I don’t see a way to extract any more information from the PR or its underlying branch from AzDO.

There was a recent issue of missing Checks for a few days, unrelated to this one. Probably that’s what you were experiencing too. I close this thread now.

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.