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

Always run Sonar scan against the master branch #921

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Conversation

lukaszlenart
Copy link
Member

No description provided.

@kusalk
Copy link
Member

kusalk commented Apr 22, 2024

I think we need to check the branch name (github.ref) and PR target branch (github.base_ref), and if either of those match 'master', run the workflow else skip it

@sepe81
Copy link
Contributor

sepe81 commented Jun 12, 2024

It would also be nice if the step "SonarCloud / Scan (push)" in the master of forked projects is skipped, as this always fails otherwise.

Error:  Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:4.0.0.4121:sonar (default-cli) on project struts2-parent: Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator to check the permissions of the user the token belongs to

@@ -44,4 +44,4 @@ jobs:
- env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
SONAR_TOKEN: ${{ secrets.SONARCLOUD_TOKEN }}
run: mvn -B -V -Pcoverage -DskipAssembly verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar --no-transfer-progress
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you removed -B b/c of https://stackoverflow.com/a/23659360 and semi-redundant --no-transfer-progress?

@sepe81
Copy link
Contributor

sepe81 commented Nov 18, 2024

@lukaszlenart Maybe rebase with master and give it another try?

Copy link
Contributor

@sepe81 sepe81 left a comment

Choose a reason for hiding this comment

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

We could use java-version: 21 on L:42

Copy link
Contributor

@sepe81 sepe81 left a comment

Choose a reason for hiding this comment

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

This looks promising. I would love to give it a try after the merge.

@sepe81
Copy link
Contributor

sepe81 commented Nov 18, 2024

One last thing: @lukaszlenart any idea how to address #921 (comment) b/c every fork also has a master branch but no sonar credentials ({"Message": "User is not authorized to access this resource with an explicit deny"}).

@lukaszlenart
Copy link
Member Author

forks

Isn't already addressed with this if on top?

if: ${{ !github.event.pull_request.head.repo.fork }}

@sepe81
Copy link
Contributor

sepe81 commented Nov 18, 2024

Isn't already addressed with this if on top?

It doesn't seem to work. The step in this run is executed in my fork, but I don't understand why.

@lukaszlenart
Copy link
Member Author

It doesn't seem to work. The step in this run is executed in my fork, but I don't understand why.

What about now?

@sepe81
Copy link
Contributor

sepe81 commented Nov 18, 2024 via email

@lukaszlenart lukaszlenart merged commit b3807b1 into master Nov 18, 2024
9 checks passed
@lukaszlenart lukaszlenart deleted the fix/sonar branch November 18, 2024 18:09
@sepe81
Copy link
Contributor

sepe81 commented Nov 18, 2024

I got a green build. Thank you so much.

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.

3 participants