we have an enforcement branch has to be up-to-date with master (base branch), so there are indeed new commits on this PR i would have seen the “updated your branch” warning, which is not the case.
so as you can see , if we are talking about PR, we now do the following:
checkout the specific sha1 of the head (including the history due to new warning of sonar on missing ref of master)
print the sha1 on the working tree
run sonar scanner
now for example one of my PRs triggered CI now on commit bb79605a21f657e59fd5c5537d6e082a960d2124, on the CI log we can see:
on checkout action log i see something like checking out ref. commit bb79605a21f657e59fd5c5537d6e082a960d2124
on git show step output i see the mentioned commit bb79605 Update readme.md
SonarScanner executed with the following command: sonarqube -Dsonar.pullrequest.branch=test-son -Dsonar.pullrequest.key=118 -Dsonar.pullrequest.base=master
so in case all good, the PR decorated successfully and we get status check success.
but as you guys saw , sometimes although the PR decorated this status check never arrives.
Thanks for the details. We should be able to reproduce and get a better understanding of what’s going on and how to fix it.
Just to be clear, the real solution for the warning about missing ref of master is fetching it (git fetch origin master). I don’t see how fetching the full history helps with that. (I don’t see a connection with the issue we’re debugging, I’m just mentioning this for the record and accuracy.)
i know, when i said history i meant we use fetch-depth: 0 which means fetch all ref for branches and tags, including master. that’s the flag on this github action.
will be happy to help with any needed information.
we are waiting for any feedback.
Didn’t had time yet to try to reproduce on my side, but i had a quick look at this analysis, and the revision analyzed was 9737bbcd4b3aecf45038c7220a7ec99809f433af
so to SumUp all these investigations, we know that:
our CI process checkout specific sha for last commit on the PR
when Sonar Analysis failed to report status, seems like the SHA that it used to analyze is the ‘future’ merge commit from the PR’s branch to master.
question is, what is the situtation when Sonar does able to report the status successfully?
it is not a merged commit? it isnt differ from the branch ref?
can you please let me know which revision analyzed on this background task ID (which passed OK) AXOPnGrOjVi1K0DNIvoa ?
We are currently investigating the issue, seems like the GH variable that should contain the SHA to analyze seem to be wrongly used on our side, we’ll keep you updated.
And to answer you question, revision is : 46cda219da3b65527d4048eb0290dd464fab3d75
ok, so also here, on a working example the revision that is being analyzed is a future commit of my Branch and master. (while the CI is checking out and running against the branch revision) so i have no clue why sometimes it works and sometimes not.
waiting for your conclusion, if there is anything we need to change on our side.
Thanks alot,
Chen
HI @clevi, we still investigating the problem but we can’t reproduce it so far. I sent a message to you requesting more information. The problem indeed seems related to the SHA we use to create the Check Runs, but the intermittent behavior is something that doesn’t combine here.
Please have a look into my message so we can make more progress on it!
i’m still puzzled from the last finding that we had 2 same scenarios with different results:
PR is running on commit X, sonar runs on commit Y, decoration does not work, status check pending.
PR is running on commit X, sonar runs on commit Y, decoration did worked, status check resolved.
although this intermittent did happened to us in the past, i wonder if its happens more since the new changes on the checkout github action.
as mentioned in the beginning we used to use checkout@v1 which basically checking out a merge commit of the PR branch and master.
from the last investigations it seems like that’s the exact thing Sonar does, no matter what actually being checked out on the source tree.
so, i wonder if reverting the checkout process to v1 , and use this merged commit, will make things work ALL the time.
i think i did checked it but to be honest, i’m already confused with all these attempts and investigations.
i still think though that sonar should analyze whats on the actual workspace tree.
waiting for your conclusion.
thanks a lot guys.
Chen
Hi, we deployed a warning that should be triggered when a PR Check Run is not decorated. The warning should be displayed at SonarCloud into the repository and branch that was analysed and looks like below:
GitHub Check Runs should be created into the merge commit, which is provided by GITHUB_SHA into the GitHub Action. To force above warning, i’ve just used the sonar.scm.revision with a commit outside the PR. If a Check Run is created for an outside commit, we got the wrong behaviour (no check at action), but from our logs looks like everything went fine, since the Check Run is actually created but but not tied to the action/PR (it became lost, not reachable into the UI, only using the API). @clevi, please can you try again and see if there is that warning? If positive, then please try again removing any commit sha reference if possible (let checkout and sonarqube use GITHUB_SHA), including sonar.pullrequest.key, sonar.pullrequest.base and sonar.pullrequest.branch (we should get it from the merge commit i guess, but let’s give it a try if you can, ok?).
@Alexandre_Holzhey, I’m having the same issue as @clevi, PRs are sometimes not decorated. I have tried your suggestion, but it didn’t seem to work. I don’t see any warnings in SC either.
Thanks to join us in this discussion. I am still working on that, but unfortunately i can’t reproduce it in a clear way. Can i ask you to please provide more information, in order to try to reproduce the issue?
Are you using GitHub Actions as well?
Your project is public? If yes, can you provide the URL of it (and skip the next questions!)?
Can you provide the GitHub Action workflow script?
The PRs that are not decorated, how many commits in average they have?
Are this PRs based on a long living branch different from “master”?
The issue appears on a PR that is based on a branch that have new commits on it? Example: PR #123 for branch “add feature” is based on branch “master”, but “master” have new commits that “add feature” don’t have.
Yes, we are using GH actions and the project is public.
The most recent issue we experienced after removing sonar.pullrequest.key , sonar.pullrequest.base and sonar.pullrequest.branch was on this PR: SC edited the comment, but the check wasn’t updated with the latest status.
The public repo of @vilkg is really insightful, especially PR#5944. I can see that:
The tip of the PR is e78c22
The scanner decorated aa98ac
= the problem. It should decorate the tip of the PR
The commit aa98ac is a merge of e78c22 into 071477
I don’t know yet where this merge comes from
071477 is the direct parent of e78c22, therefore:
This commit aa98ac adds no value whatsoever: there is no change in it, since the “merge” of e78c22 on 071477 is really the logical equivalent of e78c22 itself!
Wherever the bogus merge commit comes from, probably doesn’t matter much. The bottom line is, we want the scanner to decorate the SHA1 that is the tip of the PR.
I will track down how the scanner gets the wrong SHA1, and how to prevent that from happening.
the scenario you’ve found on the public repo of @vilkg is exactly what i was trying to show you guys.
the aa98ac commit is a merge commit of the tip of the PR and the master branch, which cames from the checkout@v2 action.
the checkout action has been changed and version2 basically checkout a merge commit of the PR and the master and not the specific head commit of the PR.
you can also see it on the log
commit aa98acaaf7cb841d4d0f9c967d9bff28be648763
Merge: 071477a3f5 e78c22fa89
Author: Gintare Vilkelyte <vilkelyte.gintare@gmail.com>
Date: Tue Aug 18 08:15:42 2020 +0000
Merge e78c22fa89b92ffa348240e84978940b9d5f5a1c into 071477a3f5da386771e7dea10d288844f250cf72
we saw it in the beginning and updated the V2 checkout action to checkout the head commit of the Pr and not the merge commit using the following
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 }} ?
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
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.
