Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: fix sonarcloud scan pr source #85

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

lpape-ionos
Copy link
Contributor

@lpape-ionos lpape-ionos commented Jan 24, 2024

Sonarcloud thinks that every branch is "main" (see https://sonarcloud.io/project/branches_list?id=ionos-cloud_cluster-api-provider-proxmox)

This is an attempt to fix that

Warning: the check run here uses the workflow from main, so I cant be sure if this works. I'd be happy to receive other suggestions

@lpape-ionos lpape-ionos force-pushed the fix-sonarcloud-branch branch 4 times, most recently from c64c910 to e0499b1 Compare January 24, 2024 15:01
@jriedel-ionos jriedel-ionos marked this pull request as ready for review January 24, 2024 15:04
@avorima
Copy link
Collaborator

avorima commented Jan 24, 2024

There are a few things to consider here.

I guess you found this page, but another page mentions sonar.branch.name and sonar.branch.target. Then the github action setup docs don't mention any of these.

The scan action seems to have access to GITHUB_EVENT_NAME, but also GITHUB_BASE_REF, GITHUB_HEAD_REF and GITHUB_REF. That should be enough to automatically fill out the required information.

Here are action logs showing the event name:

INFO: Load project pull requests
INFO: Load project pull requests (done) | time=205ms
INFO: Load branch configuration
INFO: Github event: pull_request_target
INFO: Load branch configuration (done) | time=3ms

Then there's this issue https://github.com/SonarSource/sonarqube-scan-action/issues/29 which also mentions that sonar.pullrequest.* should be populated.

I tried looking finding where these variables are used, but couldn't find anything at first glance in https://github.com/SonarSource/sonar-scanner-cli or https://github.com/SonarSource/sonar-scanner-commons

I'm not saying your change won't work, but I think the culprit actually is sonarcloud here. So I'd propose that you open an issue (or reopen the one I linked).

@lpape-ionos
Copy link
Contributor Author

I agree with you that sonar should already have all the necessary information but I dont think that this is sonar's fault. We are doing an unconventional checkout here:

- name: Checkout (preview) merge commit for PR ${{ github.event.pull_request.number }}
if: ${{ github.event_name == 'pull_request_target' }}
uses: actions/checkout@v4.1.1
with:
# Disabling shallow clone is recommended for improving relevancy of reporting
fetch-depth: 0
repository: ${{ github.event.pull_request.head.repo.full_name }}
ref: ${{ github.event.pull_request.head.ref }}

This is a workaround for branches across forks, my theory is that this confuses the sonar scan. I don't know how one would solve that properly

@wikkyk
Copy link
Collaborator

wikkyk commented Jan 25, 2024

Old thread but it doesn't seem like much has changed since and sonarcloud still doesn't support pull_request_target:
https://community.sonarsource.com/t/github-comments-stopped-working-in-github-action-after-switch-to-pull-request-target/42556/12

Comment on lines 47 to 49
args: >
-Dsonar.pullrequest.key=${{ github.event.pull_request.number }}
-Dsonar.scm.revision=${{ github.event.pull_request.head.sha }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be sufficient.

@avorima
Copy link
Collaborator

avorima commented Jan 25, 2024

Old thread but it doesn't seem like much has changed since and sonarcloud still doesn't support pull_request_target: https://community.sonarsource.com/t/github-comments-stopped-working-in-github-action-after-switch-to-pull-request-target/42556/12

Yeah, that's what I figured. I thought that opening an issue would shed some light on this

@lpape-ionos lpape-ionos merged commit 10897b9 into main Jan 25, 2024
6 checks passed
@lpape-ionos lpape-ionos deleted the fix-sonarcloud-branch branch January 25, 2024 15:32
@lpape-ionos
Copy link
Contributor Author

Looks good to me, tested in a test-PR: #91

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants