Support pull request chaining for GitHub

[GreyTeardrop]

Description

When attempting to analyze a GitHub PR that targets another PR's branch, analysis fails with an error like

No branch exists in Sonarqube with the name central-email-rate-limit

That happens as SonarQube server only stores PR analysis results in the "Pull Requests" area and does not expose it via "Branches", so none are fetched via /api/project_branches/list endpoint.

This change makes a request to the /api/project_pull_requests/list endpoint and merges its response with the response from /api/project_branches/list.

Technical details

1. I haven't spotted any constants that could be used to build PROJECT_PULL_REQUESTS_URL in the same way PROJECT_BRANCHES_URL is built.
2. PR required a reorg of the CommunityProjectBranchesLoader's test to support mocking of multiple endpoints. https://github.com/mc1arke/sonarqube-community-branch-plugin/commit/2c15a21da30ca1d22a73227714bfb332651ba0ae includes just that change without any production code changes, https://github.com/mc1arke/sonarqube-community-branch-plugin/commit/edacbe224ebe6cdfadbe27639e22c600317578f9 that makes the actual change then only needs minimal changes in the unit test.

[mc1arke]

Whilst I understand (and agree with) the premise of what you're trying to do, I don't think Sonarqube support this as a mechanism in their commercial editions, as a quick test only shows the scanner calling project_branches/list, and the server is only returning branches on this call when you check the endpoint response in https://next.sonarqube.com/.

Ideally I want to keep the plugin closely aligned to Sonarsource's implementations since we've then got to work out how to unpick any data or features if Sonarsource do go in a different direction from a design decision in the plugin at some point (e.g. saving a branch for the source of each pull request).

I'm therefore not currently looking to merge this PR (although am happy to have the community discussion in this thread about whether there a consensus on my opinion), unless anyone have evidence that Sonarqube Developer edition or above does something other than what I think it is.

[GreyTeardrop]

@mc1arke Thank you for taking a look at my PR. I can understand your reasoning here. We use PR chaining relatively frequently, so I guess I'll be maintaining a fork with this change.

To steer up the discussion - do you perhaps know how the commercial version treats PR chaining? I was only able to test SonarCloud so far, and it seems like it supports PR chains just fine, at least on GitHub. It doesn't list PR branches in the "Branches" section and only lists them in the "Pull Requests", however, PRs that target other PR branches are processed correctly.

[PauMAVA]

Hello @mc1arke and @GreyTeardrop !

I also would like to add my use case which does exactly PR chaining as explained. We have a production environment which is checked out under the main branch and we also have a pre-production (staging) environment which is usually under another branch let's say staging.

Now, If we are working on a feature (local development) on any branch we usually create a PR to the staging branch. When that is merged we try our changes in pre-production and add more features until we decide to bundle the pre-production changes to production this opening a PR stating -> main.

On this last PR there's no problem since the main branch is the default one and is present under the branches list in SonarQube. The problem arises on the first PR, the one from features to staging environment since this staging branch is listed under PRs in SonarQube. So basically we cannot implement SonarQube scans on the code that will be published in pre-production environment.

I think it would be useful to have this as a feature in this plugin, I also understand your point of view @mc1arke about not diverging from the commercial capabilities of the multiple branch feature. Maybe could we add some kind of parameter or configuration that leads to enabling this merge between the two API endpoints and that parameter could also be disabled as a default setting.

[mc1arke]

I need to do some further digging on Sonarqube's source code to see if there's any obvious feature I'm missing. I understand the use-case and agree with its value, so would like to make this work if I can.

I was only able to test SonarCloud so far, and it seems like it supports PR chains just fine

Sonarcloud is an odd deployment. It still has the concept of LONG and SHORT branches based on what I see in its API response, with that concept having vanished from other Sonarqube editions since v8.0. I'll see if I can get access to a commercial edition for testing this concept directly.

[GreyTeardrop]

@PauMAVA could you try marking staging as a long-term branch in SonarQube? I wonder if that could fix the issue for your case, as SonarQube would keep scan results for that branch in the "Branches" section then?

[PauMAVA]

@GreyTeardrop Thanks for the suggestion! Now it is listed under the branches menu but still can't find it...

INFO: Total time: 9.776s
ERROR: Error during SonarScanner execution
INFO: Final Memory: 10M/44M
ERROR: No branch exists in Sonarqube with the name 16.0-staging

I have tried with the API request and actually is being returned:

So I don't actually get why a workflow run with 16.0-staging as the target branch would fail with that error. I've been trying to backtrack the error. The message is at:

https://github.com/mc1arke/sonarqube-community-branch-plugin/blob/c9ff8091074a99dc74ded1cf68617905613c1699/src/main/java/com/github/mc1arke/sonarqube/plugin/scanner/BranchConfigurationFactory.java#L48-L51

Then from there, I backtracked the call out to the createPullRequestConfiguration, then to detectConfiguration at GithubActionsAutoConfigurer.java and from there to load method in CommunityBranchConfigurationLoader.java.

Here the method load is an override of BranchConfigurationLoader and has the conflicting ProjectBranches branches parameter. https://github.com/mc1arke/sonarqube-community-branch-plugin/blob/c9ff8091074a99dc74ded1cf68617905613c1699/src/main/java/com/github/mc1arke/sonarqube/plugin/scanner/CommunityBranchConfigurationLoader.java#L67-L68

From there the class is added at the plugin's definition: https://github.com/mc1arke/sonarqube-community-branch-plugin/blob/c9ff8091074a99dc74ded1cf68617905613c1699/src/main/java/com/github/mc1arke/sonarqube/plugin/CommunityBranchPlugin.java#L166-L167

So the class uses dependency injection. From this point, I get lost (I'm quite new to SonarQube), but I guess that it uses the CommunityProjectBranchesLoader class to populate the ProjectBranches parameter right? But then If this is true I don't get why the branch 16.0-staging wouldn't be there since in my API call we can see it being return, the same call made in the CommunityProjectBranchesLoader class...

Any idea?

[GreyTeardrop]

@PauMAVA I'm sorry, it's hard to tell. The only guess I have is that SonarQube branch settings are not retroactive - they only affect scans moving forward. Also, some CI settings might kick in here, e.g., if we're talking GitHub Actions, perhaps CI runs on the staging branch have to be triggered by push instead of pull_request triggers.

[mc1arke]

I've raised #950 which I think will do what you need - I've included a screenshot for how this looks with a test project analysing a pull request (from branch fixes) pointing at another pull request (from branch many-issues) where the target pull request (many-issues) has not had the branch scanned outside of the original pull request analysis.

[GreyTeardrop]

I've raised #950 which I think will do what you need - I've included a screenshot for how this looks with a test project analysing a pull request (from branch fixes) pointing at another pull request (from branch many-issues) where the target pull request (many-issues) has not had the branch scanned outside of the original pull request analysis.

Interesting! From your description, I'd expect that the stacked PR would include all the issues from the base PR (as it's compared against the base branch) but your screenshot clearly indicates the opposite. Is that something SonarQube does by itself?

[mc1arke]

Yes, Sonarqube appears to be reporting any new issues introduced directly in the PR, and filters out the issues previously introduced in the target PR, but also doesn't report the fixed issues in the counter when #948 is applied, so is an odd mix of includes and excludes. Some testing from other users (and reporting it on #980) would be useful.

[mc1arke]

Closing this since an alternative implementation was including in the last release - feel free to give it test and provide any feedback.