PR based on master but targeting Staging shows Staging issues outside the PR

SonarQube
1

Branch Configuration

I don’t have a good way to produce a diagram of my actual commits, so I’m modifying an existing diagram:
branch diagram with master, release, develop, and feature branches
SOURCE: https://stackoverflow.com/a/20755706/154527

Master, Release, and Develop are long-lived branches.

I have PR-1234 which is at commit L in the diagram. I have Sonar scans for A, B, C, D, E, and F.

The PR targets Develop.

What I expect

I expect it to use the scan at E and consider only K and L as new code/leak on this PR branch.

What actually happens on 7.3 and earlier

A different scan of Develop appears to be used and other commits outside K and L are considered.

2

What actually happens on 7.4 and later

SonarQube uses the scan at E and consider only K and L as new code/leak on this PR branch.

I have validated the heck out of this. I tried to break it and couldn’t. I’ve tested rebasing on multiple previous (and scanned) versions of Develop. I’ve tested rebasing on versions of Release and Master that are not scanned, but have direct descendants on Develop that are.

This is some awesome work, @ganncamp! Wow.

Conclusion

If you are encountering this issue, you should consider upgrading to 7.4 and making sure that the base branch is long-lived and scanned on each commit.

Tested version

SonarQube Enterprise 7.4.0.18908
Plugins: Git 1.6.0.1349
SonarQube Scanner 3.2.0.1227

4 Likes
3

Thanks @AlainODea for taking the time and effort for the extensive (re-)testing, and for the kind words!
(Awesome diagram-hack too, making the thread highly informative and insightful in a very effective way.)

1 Like
4

Thanks for the thorough testing and the back pats Alain. They’re nice to get!

5

There still is one odd case here.

There are Sonar scans for Develop at D, E, and F.
I have PR-4321 at L.
F is merged to Develop including fixes for existing Sonar issues I-123 and I-321 on Develop.
The Sonar scan for PR-4321 shows I-123 and I-321 as introduced by PR-4321.

I would expect the PR-4321 scan to diff against the scan at E, rather than the scan at F.

Is that expected?

6

Hi,

This is expected. The issue exists in the PR and not it’s base. The assumption is that you’ll either rebase to pick up those fixes (which you’ll need to do before merge anyway) or merge your PR/SLB before it becomes a problem.

 
Ann

1 Like
7

Thank you. That makes sense.

8

While this makes sense and it’s good to know, it will complicate our process.

Merges to the equivalent of Develop happen multiple times a day and developers understandably don’t want to merge or rebase work in progress multiple times a day to keep up with that.

Most of our developers don’t rebase prior to merge. This has rarely caused issues and would be a substantial process change for us. If they rebase, all reviews are dismissed, and they need to wait 40 minutes for the regression suite to run. If a new merge gets into Develop in the mean time, they’re back to square one.

Will a future version of SonarQube use the scan at the branchpoint (E) instead of the latest scan on the base branch as the reference for new issues?

9

Hi Alain,

No. We can only compare to current state, not previous states.

At the same time, I have to ask: why not rebase before L is analyzed? Surely L triggered the regression suite on its own? And if they simply don’t want to rebase, then just mark those issues as Won’t Fix.

 
Ann

10

No. We can only compare to current state, not previous states.

Okay. I’ll communicate this limitation and work out the process change.

At the same time, I have to ask: why not rebase before L is analyzed?

Habit. There’s not really a good reason not to rebase. The analysis happens automatically when the PR is opened or new code is pushed.

Surely L triggered the regression suite on its own?

It did. The trouble is that people sometime open the PR after F is merged. I’ll see about changing that practice.

And if they simply don’t want to rebase, then just mark those issues as Won’t Fix.

That is what I have recommended and will reiterate now.

I appreciate the clarity here. I’ll work on getting our processes and habits changed to adapt to this.