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: Integrate SonarCloud #573

Conversation

rettichschnidi
Copy link
Contributor

@rettichschnidi rettichschnidi commented Apr 2, 2021

This will give us details coverage and static and static code analysis on every code push, which includes the most recent commit in master. It also allows us to enforce certain code quality attributes.

Example analysis of the current master:

Please note:

  • If/when this PR got merged...
    • we should have a look at all findings
    • we could add shiny badges in the top-level readme :)
  • In a follow up commit I intend to implement a way of running the scans on PRs from committers and, if possible in a secure fashion, to allow running the analysis on 3rd-party PRs too (as described in Keeping your GitHub Actions and workflows secure: Preventing pwn requests for inputs).

To enable the integration for your own fork, take the following steps:

@rettichschnidi rettichschnidi changed the title ci: Integrate SonarCloud [RFC] ci: Integrate SonarCloud Apr 2, 2021
@rettichschnidi
Copy link
Contributor Author

@qleisan @mlasch @sbernard31 @sbertin-telular Would like to hear your opinions on this.

@rettichschnidi rettichschnidi changed the title [RFC] ci: Integrate SonarCloud ci: Integrate SonarCloud Apr 5, 2021
@sbernard31
Copy link
Contributor

The checks can only work if/when @sbernard31 enables SonarQube as mentioned above

I never did that but if we want to I guess I need to ask for permission to eclipse webmaster ( see https://bugs.eclipse.org/bugs/show_bug.cgi?id=572407)

Would like to hear your opinions on this.

No strong opinion on it. I didn't play so much with sonar, so I guess I can not give you relevant feedback.
I contributed to few projects where it was used and some time feedback returned was annoying but I guess this is more setting issue. So maybe the question could be : is it no too much time consuming to set it to report really useful feedback ?

@rettichschnidi
Copy link
Contributor Author

I contributed to few projects where it was used and some time feedback returned was annoying but I guess this is more setting issue. So maybe the question could be : is it no too much time consuming to set it to report really useful feedback ?

I think there are two aspects:

  1. Code coverage: Very easy to set up and maintain - just specify the code coverage level needed to allow a PR to be merged. Proposing to get this integrated in the merge workflow soon-ish.
  2. Static code analysis: More work to set up initially, will have some maintenance cost for reviewing the findings. In my opinion the benefits of having this analysis nevertheless outweigh the efforts needed and I am willing to take care of it (initially and later on). However, I am not a committer, therefore I can not do this due to the lack of access. We could still have the analysis enabled but not enforcing it on PRs though.

@rettichschnidi
Copy link
Contributor Author

I just rebased the branch. Now that I have committer permission, I think I am able to open the relevant issues in the Eclipse Bugzilla myself.

@sbernard31 Given I am taking care of the initial (just me) and ongoing maintenance (adjusting rules, help with findings in PRs, etc.) would you be OK with moving forward?

@sbernard31
Copy link
Contributor

Now that I have committer permission

🎉

@rettichschnidi should you get feedback from other members of the team before to open bugzilla ?

@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Apr 14, 2021

@rettichschnidi should you get feedback from other members of the team before to open bugzilla ?
👍

@qleisan @mlasch @sbernard31 @sbertin-telular Any feedback regarding my proposal?

@qleisan
Copy link

qleisan commented Apr 16, 2021

To enable the integration for your own fork, take the following steps:
Log in to https://sonarcloud.io/ using your GitHub account
Visit https://sonarcloud.io/projects/create, add Wakaama (fairly
self-explanatory)
Create a PR in your repository to verify the integration

Pushed this branch to my wakaama fork repo and got sonarcloud to execute (after setup)
Check out the result here

SonarCloud Quality Gate failed.

Bug C 3 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

99.8% 99.8% Coverage
0.0% 0.0% Duplication

@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Apr 16, 2021

The findings do already exist in the master code, but since master never got scanned, those show up now for the first time, causing the PR to get rejected.

Looking good (as expected) to me.

I intend to triage those findings in the master branch (in SonarCloud), but can do so only once this PR got merged.

Copy link

@qleisan qleisan left a comment

Choose a reason for hiding this comment

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

LGTM. I'm in favor of using sonarcloud

Copy link
Contributor

@mlasch mlasch left a comment

Choose a reason for hiding this comment

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

Looks very promising 👍

tools/ci/run_ci.sh Outdated Show resolved Hide resolved
@rettichschnidi
Copy link
Contributor Author

@sbernard31 We are facing some issues with the setup of SonarCloud. Can I please get your +1 so that I can get admin temporary permissions on this repository to debug/fix the issue?

@sbernard31
Copy link
Contributor

@rettichschnidi, done.

Sorry for the delay but I was out of office yesterday. By the way this will probably happen often during april/may because I will work something like 3 days by week during this period. (too many day off accumulated 😬 )

This will give us details coverage and static and static code analysis
for every push.

To enable the integration, take the following steps:
 - Log in to https://sonarcloud.io/ using your GitHub account
 - Visit https://sonarcloud.io/projects/create, add Wakaama (fairly
   self-explanatory)
 - Create a PR in your repository to verify the integration

PRs do not get checked because it is tricky to do it in a secure
fashion:
 - https://jira.sonarsource.com/browse/MMF-1371
 - https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Signed-off-by: Reto Schneider <code@reto-schneider.ch>
@rettichschnidi
Copy link
Contributor Author

Sorry for the delay but I was out of office yesterday. By the way this will probably happen often during april/may because I will work something like 3 days by week during this period. (too many day off accumulated grimacing )

Not a problem at all - enjoy your time!

@rettichschnidi
Copy link
Contributor Author

rettichschnidi commented Apr 20, 2021

Turns out, that running SonarCloud for arbitrary external PRs can not be done in a secure fashion [1][2].

I will change my PR to run the analysis only on commits, not on PRs. I might come up with an improvement which allows committers to selectively run PRs using a combination of pull_request_target [3] and labels [2], but this will take a while, will be a separate PR.

[1] https://jira.sonarsource.com/browse/MMF-1371
[2] https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
[3] https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target

@rettichschnidi rettichschnidi marked this pull request as ready for review April 20, 2021 13:35
Found by SonarCloud.

Signed-off-by: Reto Schneider <code@reto-schneider.ch>
SonarCloud has complained before.

Signed-off-by: Reto Schneider <code@reto-schneider.ch>
As found by SonarCloud.

Signed-off-by: Reto Schneider <code@reto-schneider.ch>
@rettichschnidi rettichschnidi merged commit 48c933b into eclipse-wakaama:master Apr 20, 2021
@rettichschnidi rettichschnidi deleted the gardena/rs/improve-ci-6-sonar branch April 20, 2021 23:40
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

4 participants