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

chore: add ci analysis for sonar #337

Merged
merged 17 commits into from
Mar 28, 2024
Merged

chore: add ci analysis for sonar #337

merged 17 commits into from
Mar 28, 2024

Conversation

bmunguli
Copy link
Contributor

@bmunguli bmunguli commented Feb 21, 2024

Description

  • Run CI analysis with sonar instead of automatic analysis which gives the opportunity for publishing the test coverage results in sonar cloud
  • Use maven plugin to integrate with sonar as this is the recommended way
  • Add jacoco to produce test coverage reports and show results in sonarcloud

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

@bmunguli
Copy link
Contributor Author

bmunguli commented Feb 22, 2024

Can someone with sonar cloud access rights disable the automatic analysis on sonar so the cli integration can be fully tested if this PR looks good and we would like to merge it? Currently the pipeline is failing because of conflicts between automatic analysis and ci analysis.
Refs: https://docs.sonarsource.com/sonarcloud/advanced-setup/automatic-analysis/#conflict-with-ci-based-analysis

pom.xml Outdated

<!-- build -->
<maven.compiler.version>3.8.1</maven.compiler.version>


Choose a reason for hiding this comment

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

Please remove the second empty line, one empty line should be enough.

Copy link

sonarcloud bot commented Mar 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@tuncaytunc-zf tuncaytunc-zf left a comment

Choose a reason for hiding this comment

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

LGTM

@tunacicek
Copy link
Contributor

Hi @bmunguli @tuncaytunc-zf ,
the build failed. Do you have the possibility to see the logs?

@bmunguli
Copy link
Contributor Author

bmunguli commented Mar 5, 2024

Can someone with sonar cloud access rights disable the automatic analysis on sonar so the cli integration can be fully tested if this PR looks good and we would like to merge it? Currently the pipeline is failing because of conflicts between automatic analysis and ci analysis.
Refs: https://docs.sonarsource.com/sonarcloud/advanced-setup/automatic-analysis/#conflict-with-ci-based-analysis

@tunacicek thanks for reviewing this, as mentioned in my previous comment the pipeline is currently failing because of conflicts between automatic analysis and ci analysis analysis enabled in sonarcloud. I need someone with access rights in tractus-x sonarcloud project https://sonarcloud.io/project/overview?id=eclipse-tractusx_sldt-digital-twin-registry to disable the configured automatic analysis so the ci analysis could work.

From the logs:

Error: Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.10.0.2594:sonar (default-cli) on project digital-twin-registry: You are running CI analysis while Automatic Analysis is enabled. Please consider disabling one or the other. -> [Help 1]

Refs: https://docs.sonarsource.com/sonarcloud/advanced-setup/automatic-analysis/#conflict-with-ci-based-analysis

@tunacicek
Copy link
Contributor

Can someone with sonar cloud access rights disable the automatic analysis on sonar so the cli integration can be fully tested if this PR looks good and we would like to merge it? Currently the pipeline is failing because of conflicts between automatic analysis and ci analysis.
Refs: https://docs.sonarsource.com/sonarcloud/advanced-setup/automatic-analysis/#conflict-with-ci-based-analysis

@tunacicek thanks for reviewing this, as mentioned in my previous comment the pipeline is currently failing because of conflicts between automatic analysis and ci analysis analysis enabled in sonarcloud. I need someone with access rights in tractus-x sonarcloud project https://sonarcloud.io/project/overview?id=eclipse-tractusx_sldt-digital-twin-registry to disable the configured automatic analysis so the ci analysis could work.

From the logs:

Error: Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.10.0.2594:sonar (default-cli) on project digital-twin-registry: You are running CI analysis while Automatic Analysis is enabled. Please consider disabling one or the other. -> [Help 1]

Refs: https://docs.sonarsource.com/sonarcloud/advanced-setup/automatic-analysis/#conflict-with-ci-based-analysis

@bmunguli : Thanks for your feedback.
@Siegfriedk, @RoKrish14 : Could one of you please disable the configured automatic analysis?

@tunacicek
Copy link
Contributor

@bmunguli : Could you please update the dependencies file? See also trg-7-04
https://eclipse-tractusx.github.io/docs/release/trg-7/trg-7-04/#checking-libraries-using-the-eclipse-dash-license-tool

You can run the Eclipse Dash License Tool:
https://github.com/eclipse/dash-licenses/blob/master/README.md

@bmunguli
Copy link
Contributor Author

bmunguli commented Mar 5, 2024

@tunacicek running the dash tool via maven plugin locally updates the DEPENDENCIES file with some libraries i have not introduced as part of this PR. The two new plugins that are introduced: jacoco and sonar are not reflected because they are not dependencies.
Screenshot 2024-03-05 at 11 13 22

Should I skip including those changes as part of this PR?

@tunacicek
Copy link
Contributor

@tunacicek running the dash tool via maven plugin locally updates the DEPENDENCIES file with some libraries i have not introduced as part of this PR. The two new plugins that are introduced: jacoco and sonar are not reflected because they are not dependencies. Screenshot 2024-03-05 at 11 13 22

Should I skip including those changes as part of this PR?

@bmunguli : yes, you can skip those changes. thank you

@Siegfriedk
Copy link

@tunacicek i disabled the automated analysis

@tunacicek
Copy link
Contributor

@tunacicek i disabled the automated analysis

@Siegfriedk :Thank you :)

- passing project key in pom.xml seems to cause
@bmunguli
Copy link
Contributor Author

bmunguli commented Mar 6, 2024

@tunacicek can you please check if the SONAR_TOKEN is set as a secret on this repository or organisation level?

@tunacicek
Copy link
Contributor

@tunacicek can you please check if the SONAR_TOKEN is set as a secret on this repository or organisation level?

@bmunguli i have no permission to check it.
@Siegfriedk : Could you please check if the token is set?

@bmunguli : A reference implementation can be found here:
https://github.com/eclipse-tractusx/traceability-foss

@Siegfriedk
Copy link

@tunacicek only EF can check it. For adhoc, you can ask frederric gurr per matrix and hope he doesn't bite you :)

@tunacicek
Copy link
Contributor

@Siegfriedk thanks, I will try it :)

@fredg02
Copy link

fredg02 commented Mar 6, 2024

Neither a repo-level (for sldt-digital-twin-registry) nor an org-level SONAR_TOKEN secret exists.

@fredg02
Copy link

fredg02 commented Mar 6, 2024

@tunacicek only EF can check it. For adhoc, you can ask frederric gurr per matrix and hope he doesn't bite you :)

1:1 communication does not scale. So please rather mention me in a comment here or create a HelpDesk issue.

@tunacicek
Copy link
Contributor

Neither a repo-level (for sldt-digital-twin-registry) nor an org-level SONAR_TOKEN secret exists.

@fredg02 : thanks for your feedback. I would create a ticket to have a token.

@tunacicek
Copy link
Contributor

tunacicek commented Mar 6, 2024

@tunacicek
Copy link
Contributor

@bmunguli : I created the ticket https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4399

@bmunguli ticket is closed. SONAR_TOKEN is available now. I rerun the build

GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any
SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
run: mvn -B --batch-mode sonar:sonar -Dsonar.projectKey=eclipse-tractusx_sldt-digital-twin-registry

Choose a reason for hiding this comment

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

I think the -Dsonar.organization=eclipse-tractusx parameter is missing here.

Copy link
Contributor Author

@bmunguli bmunguli Mar 8, 2024

Choose a reason for hiding this comment

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

The organization its defined in pom.xml , will add in workflow too and see

@bmunguli
Copy link
Contributor Author

bmunguli commented Mar 8, 2024

@tunacicek from the logs its complaining that:
failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.10.0.2594:sonar (default-cli) on project digital-twin-registry: Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator

Double Checking the projectKey and organization look correct to me and matching with the project in sonar.
The SONAR_TOKEN is usually bound to a user, could it be that the user who generated the token does not have permissions to perform analysis on this project?

@tunacicek
Copy link
Contributor

@tunacicek from the logs its complaining that: failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.10.0.2594:sonar (default-cli) on project digital-twin-registry: Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator

Double Checking the projectKey and organization look correct to me and matching with the project in sonar. The SONAR_TOKEN is usually bound to a user, could it be that the user who generated the token does not have permissions to perform analysis on this project?

Hi @pstankie, Could you check it please?

@pstankie
Copy link

pstankie commented Mar 8, 2024

I added SONAR_ORGANIZATION and SONAR_PROJECT_KEY github variables to repository settings. You can use them:

orgs.newRepoVariable('SONAR_ORGANIZATION') {
      value: "eclipse-tractusx",
    },
    orgs.newRepoVariable('SONAR_PROJECT_KEY') {
      value: "eclipse-tractusx_sldt-digital-twin-registry",
    },

@tunacicek
Copy link
Contributor

I added SONAR_ORGANIZATION and SONAR_PROJECT_KEY github variables to repository settings. You can use them:

orgs.newRepoVariable('SONAR_ORGANIZATION') {
      value: "eclipse-tractusx",
    },
    orgs.newRepoVariable('SONAR_PROJECT_KEY') {
      value: "eclipse-tractusx_sldt-digital-twin-registry",
    },

@pstankie :Thank you very much for your support.
@bmunguli : Could you please use the variables instead of hard coded values.

@bmunguli
Copy link
Contributor Author

@tunacicek i tried the same configuration with a private sonar cloud instance and the integration works fine.
My only guess here why its not working on this PR is the sonar token which is out of my control. Can we please review that token or generate a new one and make sure it has Execute Analysis permissions?

@pstankie
Copy link

I updated SONAR_TOKEN. Can you try again ?

- it seems accessing the values from variables does not work
@pstankie
Copy link

So the SONAR_TOKEN is not visible in PR from forked repository. You may try pull_request_target trigger in this specific case.

@bmunguli
Copy link
Contributor Author

@pstankie thank you for your feedback here, will check the triggers

@bmunguli
Copy link
Contributor Author

bmunguli commented Mar 18, 2024

@tunacicek as @pstankie commented previously github does not allow access to secrets on forked PRs so the sonar analysis can not happen there. I asked the question on the community. Seems there are few options recommended:

I am not super confident about the setup of the first two, shall we go ahead and use same approach as traceability-foss for now (the drawback with this is that we can not get feedback if the integration works until changes got to main)?

@tunacicek
Copy link
Contributor

@tunacicek as @pstankie commented previously github does not allow access to secrets on forked PRs so the sonar analysis can not happen there. I asked the question on the community. Seems there are few options recommended:

I am not super confident about the setup of the first two, shall we go ahead and use same approach as traceability-foss for now (the drawback with this is that we can not get feedback if the integration works until changes got to main)?

Hi @bmunguli ,
yes, we can do it like in traceability and then check whether it works or not. Could you fix the merge conflicts?

@bmunguli
Copy link
Contributor Author

@tunacicek merged main into the branch and added the soanar changes on a dedicate workflow same as tarceability repo

Copy link
Contributor

@tunacicek tunacicek left a comment

Choose a reason for hiding this comment

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

LGTM

@tunacicek tunacicek merged commit bee47ef into eclipse-tractusx:main Mar 28, 2024
7 checks passed
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.

6 participants