kingoleg
commented
6 years ago - Updated version to 1.4.0-SNAPSHOT
- Added some more logs
- Exclude issues not related to PR Diff
- Fixed slf4j test binding
Open kingoleg opened 6 years ago
kingoleg
commented
6 years ago kingoleg
commented
6 years ago The motivation:
Sonar discontinued the "incremental" analysis mode.
So now we how only "issues" analysis mode.
The difference between the mode is that "incremental" check only added/changes lines. Where "issues" mode is full code scan just without storing issues to a sonar server. It means, that if there is a file A with sonar issues in a branch, and I create a PR the branch with adding other file B without issues, sonar "issues" mode still will return issues for A. But as A is not a part of the PR, we are not interesting in this issue. So we should skip all issues not related to files in PR diff.
Note. Other bitbucket plugin (https://github.com/mibexsoftware/sonar-bitbucket-plugin + the same for server bitbucket) goes other way: it constructs a list of paths changed in diff and set "sonar.includes" to force sonar to scan only changed files. It can speed up sonar analysis little bit, especially for big projects. But it's not easy to integrate this behavior to sonar-stash
t-8ch
commented
6 years ago Hi @kingoleg,
I'll take a look at it.
One note about 3): Deciding if an issue should be included or not can only be done properly based on the information given by SQ. The fact that the issue falls into the git diff is not a good indicator for that.
kingoleg
commented
6 years ago @t-8ch
Regarding 3)
Please check the motivation comments.
t-8ch
commented
6 years ago To clarify: In the situation you describe, you want to merge B into A while the normal SQ analysis has been performed on master?
t-8ch
commented
6 years ago Always ignoring issues outside of the diff will still lead to problems when changes to one file affect the issues of another file. Example:
Foo.java is tested in FooTest.java.FooTest.java is modified, dropping test coverage accidentally.Foo.javaFoo.java, as it does not belong to the diff, throwing away information.kingoleg
commented
6 years ago I agree with you, that it can be some unreported new issues because of this. But it can be in any case.
For example, I have a PR where an issues on new added line is reported by sonar as not new issue. Sonar just creates the same issue.key as has an other issue on the same file but for not touched line.
Or, for example, I have a sonar java check that is completely ignored on sonar.analysis.mode = issues but reported fine on full branch sonar review.
So, I have no doubt that only sonar PR review is enough to avoid new sonar violations.
The issue that I have, the target branch can be changed after full sonar scan and then PR branch review scan can see the issue related to a file that is merged with other PR, but sonar will still report this file and this issues. And this is mistake.
I cannot guaranty that when sonar PR review is started, the target branch sonar full scan is done and the branch is not changed after
t-8ch
commented
6 years ago Hi @kingoleg,
sorry for the long delay.
About your two points:
The issue key is based on an UUID, that incorporates:
It is extremely unlikely, that there will be a collision in the issue.key.
If a check only runs in full scan mode, then it there should be no false positives with sonar-stash. First this hypothetical check runs and creates in issue. Then the issue scan runs with sonar-stash and the issue is not new, so sonar-stash will ignore it.
I understand your issue with the ordering and configuration of the scans. However in my opinion this should be addressed by the system managing the scans in the first place. (I acknowledge, that this is most probably hard to implement)
Any workaround in sonar-stash will be limited to sonar-stash. It will break apart as soon as you want to use more plugins.
In contrast the workaround will lead to unexpected behaviour for users whose setup is "correct".