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

Add SonarQube Analysis for PRs #26993

Merged
merged 30 commits into from
Aug 22, 2024
Merged

Conversation

anarsultanov
Copy link
Contributor

@anarsultanov anarsultanov commented Aug 17, 2024

This PR introduces a custom GitHub Action to address Issue #19467 by adding SonarQube reports to PRs, enabling the detection of newly introduced issues and helping maintain high code quality in the project.

Summary:

  • Implementation: Introduces a GitHub Action that runs SonarQube analysis using Docker.
  • Caching Mechanism: On pushes to the main branch, the entire generated application is cached. This cache is then restored during PR analysis.
  • PR Analysis: The cached main branch project is analyzed first, followed by the analysis of the PR changes. This process allows to identify and report only the issues newly introduced in the PR.

image

Simplification Suggestion:

We could simplify the approach by analyzing the generated project directly instead of focusing on the changes. This would streamline the workflow, eliminate the dependency on caching, and allow the use of the official SonarQube Community Edition image.


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

  • the sonar container version should be reproducible and be loaded from generator-jhipster provided version instead of using latest.
    A new release will introduce errors/bugs without warnings.
  • new issues is only reported after a period. AFAIK there is no way to check new issues between 2 checks. the pr is simulating a PR so it may work.
  • PR comments requires a token to work.
  • code coverage won't work without caching tests result.

@mshima
Copy link
Member

mshima commented Aug 19, 2024

We could simplify the approach by analyzing the generated project directly instead of focusing on the changes. This would streamline the workflow, eliminate the dependency on caching, and allow the use of the official SonarQube Community Edition image.

JHipster Lite is quite different.
JHipster generates MUCH more code.
A couple of months ago we did some work to reduce sonar issues/bugs and we got it down to 12 issues, a new sonar release and the number jumped to more than 100 issues, we reduced to 12 again, another sonar release and 300 issues are reported.
Targeting 0 issues is not doable for us.
The goal is to don't introduce new issues and make easier to check if issues are been fixed by a PR.

@anarsultanov
Copy link
Contributor Author

anarsultanov commented Aug 19, 2024

Thank you for the review and comments, @mshima.

  • SonarQube Version: I agree with using a specific SonarQube version instead of latest to ensure reproducibility and avoid unexpected issues with new releases. However, I’m not entirely clear on how generator-jhipster should provide a version to the action. Could you clarify that part?

  • PR Simulation: Exactly, we simulate pull request analysis using a plugin that allows this in the Community version of SonarQube, which makes it feasible in our setup.

  • PR Comments and Token Requirement: You’re absolutely right about the token requirement. As I understand it, even the default GITHUB_TOKEN won’t work for comments when the workflow is triggered by forks. We would need either a PAT (Personal Access Token) or a GitHub App-generated token. If that’s not feasible, we might consider removing the comment feature and relying solely on the logs.
    Update: Another options seems to be to use the pull_request_target event to ensure the workflow has the necessary permissions to comment on PRs. However, this approach also requires extra caution to avoid executing untrusted code.

  • Code Coverage and Caching: Test results are cached along with the entire project after all tests have been run, ensuring that code coverage is properly handled.

@mshima
Copy link
Member

mshima commented Aug 20, 2024

  • SonarQube Version: I agree with using a specific SonarQube version instead of latest to ensure reproducibility and avoid unexpected issues with new releases. However, I’m not entirely clear on how generator-jhipster should provide a version to the action. Could you clarify that part?

Sonar version should be get from

FROM sonarqube:10.6.0-community

Like we get node/java/npm versions and build ci matrix with them.

@mshima
Copy link
Member

mshima commented Aug 20, 2024

  • PR Comments and Token Requirement: You’re absolutely right about the token requirement. As I understand it, even the default GITHUB_TOKEN won’t work for comments when the workflow is triggered by forks. We would need either a PAT (Personal Access Token) or a GitHub App-generated token. If that’s not feasible, we might consider removing the comment feature and relying solely on the logs.
    Update: Another options seems to be to use the pull_request_target event to ensure the workflow has the necessary permissions to comment on PRs. However, this approach also requires extra caution to avoid executing untrusted code.

pull_request_target should be avoided.
We should try a Fine-grained PAT with ISSUES and PR comment only.

@DanielFran can you generate a Fine grained PAT with PR and ISSUES (to be used in https://github.com/jhipster/generator-jhipster/blob/main/.github/workflows/issue-check.yml workflow) comment permission?

@anarsultanov
Copy link
Contributor Author

  • SonarQube Version: I agree with using a specific SonarQube version instead of latest to ensure reproducibility and avoid unexpected issues with new releases. However, I’m not entirely clear on how generator-jhipster should provide a version to the action. Could you clarify that part?

Sonar version should be get from

FROM sonarqube:10.6.0-community

Like we get node/java/npm versions and build ci matrix with them.

To be able to analyze differences (using pull request analysis), we have to use the mc1arke/sonarqube-with-community-branch-plugin image, as the standard SonarQube Community Edition does not support this feature. Should we consider adding another entry to the Dockerfile for this specific image, or is there a preferred approach to handle this?

@mshima
Copy link
Member

mshima commented Aug 20, 2024

  • SonarQube Version: I agree with using a specific SonarQube version instead of latest to ensure reproducibility and avoid unexpected issues with new releases. However, I’m not entirely clear on how generator-jhipster should provide a version to the action. Could you clarify that part?

Sonar version should be get from

FROM sonarqube:10.6.0-community

Like we get node/java/npm versions and build ci matrix with them.

To be able to analyze differences (using pull request analysis), we have to use the mc1arke/sonarqube-with-community-branch-plugin image, as the standard SonarQube Community Edition does not support this feature. Should we consider adding another entry to the Dockerfile for this specific image, or is there a preferred approach to handle this?

A specific Dockerfile in test-integration/sonar-pr then?
It's possible to use the Dockerfile instead of passing the version through matrix.

@anarsultanov
Copy link
Contributor Author

A specific Dockerfile in test-integration/sonar-pr then? It's possible to use the Dockerfile instead of passing the version through matrix.

I'm not entirely sure I understand the requirement. Do you mean creating a Dockerfile in test-integration/sonar-pr that inherits from the specific image (e.g., FROM mc1arke/sonarqube-with-community-branch-plugin:10.3-community), which we then build and start during the workflow? I’m wondering what the benefit of this approach would be compared to simply specifying the version directly in the action.

@mshima
Copy link
Member

mshima commented Aug 20, 2024

A specific Dockerfile in test-integration/sonar-pr then? It's possible to use the Dockerfile instead of passing the version through matrix.

I'm not entirely sure I understand the requirement. Do you mean creating a Dockerfile in test-integration/sonar-pr that inherits from the specific image (e.g., FROM mc1arke/sonarqube-with-community-branch-plugin:10.3-community), which we then build and start during the workflow? I’m wondering what the benefit of this approach would be compared to simply specifying the version directly in the action.

Reproducible version and auto update through dependabot.

@mshima
Copy link
Member

mshima commented Aug 20, 2024

A docker-compose file with renovate auto update is an alternative.

@anarsultanov
Copy link
Contributor Author

Reproducible version and auto update through dependabot.
A docker-compose file with renovate auto update is an alternative.

Thanks for the clarification. We could use a custom manager if Renovate doesn’t detect the dependency in the action, but adding a Dockerfile might be simpler. I’ll push this change together with the token once it’s created by @DanielFran

@DanielFran
Copy link
Member

DanielFran commented Aug 20, 2024

ISSUES and PR comment only

@mshima @anarsultanov I created JHIPSTER_PAT_PR_ISSUES token

image

@mshima
Copy link
Member

mshima commented Aug 20, 2024

@DanielFran have you added to repository secrets?

@DanielFran
Copy link
Member

@DanielFran have you added to repository secrets?

Yes, it can be used PAT_PR_ISSUES_TOKEN

test-integration/sonar-pr/docker-compose.yml Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

  • if cache does not match, the overall issues should be shown.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

use kebab case instead of snake case. It's the default for actions.

.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
renovate.json Outdated Show resolved Hide resolved
anarsultanov and others added 2 commits August 21, 2024 22:18
Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
@mshima
Copy link
Member

mshima commented Aug 21, 2024

Dockerfile needs to be added to dependabot config:

- package-ecosystem: 'docker'
directory: '/generators/server/resources/'
schedule:
interval: 'daily'
time: '00:20'
open-pull-requests-limit: 5
labels:
- 'theme: dependencies'
- 'theme: docker :whale:'
- 'skip-changelog'

@anarsultanov
Copy link
Contributor Author

anarsultanov commented Aug 21, 2024

Seems like some issue with token: Error: Parameter token or opts.auth is required
And based on this, it might not even be possible to make it work with:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

One option we can try is to load the result into artifacts since it uses some other token that might be accessible.

Update: but if you want to keep things as is, to only get comments on pull requests created directly in the repository, we can get rid of the PAT and rely on the default token.

anarsultanov and others added 2 commits August 21, 2024 23:27
Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Polish

.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/workflows/angular.yml Outdated Show resolved Hide resolved
Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Fold log to improve readability.

.github/actions/sonar/action.yml Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Show resolved Hide resolved
.github/actions/sonar/action.yml Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
.github/actions/sonar/action.yml Outdated Show resolved Hide resolved
@mshima mshima enabled auto-merge August 22, 2024 00:33
@mshima mshima merged commit d1415ad into jhipster:main Aug 22, 2024
52 checks passed
@DanielFran DanielFran mentioned this pull request Aug 22, 2024
1 task
@anarsultanov anarsultanov deleted the issue-19467-sonar branch August 23, 2024 10:40
@anarsultanov
Copy link
Contributor Author

Bounty claimed https://opencollective.com/generator-jhipster/expenses/217026

@DanielFran
Copy link
Member

@anarsultanov approved

@mraible mraible added this to the 8.7.0 milestone Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants