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

RATIS-940. Add sonar check for ratis #108

Merged
merged 3 commits into from May 27, 2020
Merged

Conversation

dineshchitlangia
Copy link
Contributor

Ratis instance has been created in ASF's Sonar Cloud. This can now be used to analyze and improve the code quality. To ensure the analysis is automatically executed whenever there is a commit in master branch, we need to add a github action for Sonar check.
This PR aims to do that.

https://issues.apache.org/jira/browse/RATIS-940

For testing, I was able to add a token in my fork on incubator-ratis and confirm the script triggers analysis. For the incubator-ratis repo, ASF INFRA team helped to add the sonar cloud token in the repo settings via https://issues.apache.org/jira/browse/INFRA-20295
Once this PR is merged, I can confirm that the sonar check got enabled correctly.

@dineshchitlangia
Copy link
Contributor Author

@elek Could I request your quick review as you had done the similar changes in Ozone?
Unlike your changes in Ozone, I am performing the sonar check first and then the unit tests as Ratis has some broken UTs. Those will ensure the unit check part exits out once it fails the unit tests and may not trigger the sonar check.

@dineshchitlangia
Copy link
Contributor Author

Checkstyle failure is unrelated to the PR.

ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java
47: Unused import - org.apache.ratis.server.raftlog.RaftLog.INVALID_LOG_INDEX.

Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

+1 LGTM Thanks to drive this effort @dineshchitlangia

GITHUB_TOKEN is not required in the env section as far as I know, but doesn't cause any problem.

Can be removed if you merge it locally and push it...

@dineshchitlangia
Copy link
Contributor Author

@elek Thank you for the review. You are right the Github token is not needed when merging/pushing locally. As we are slowly moving towards merging directly via GitHub PRs this will be needed, at least as per Sonar's documentation :)

Let me know if you can commit this or you want me to drop the github token.

@elek
Copy link
Member

elek commented May 27, 2020

this will be needed, at least as per Sonar's documentation :)

Ok, thanks. Good to know. In that case, let me merge it.

@elek elek merged commit 00f1747 into apache:master May 27, 2020
@dineshchitlangia dineshchitlangia deleted the RATIS-940 branch May 27, 2020 14:50
@dineshchitlangia
Copy link
Contributor Author

Thanks @elek for review/commit.
Thanks @mukul1987 for reviews.

I confirmed we are able to see the Sonar analyses : https://sonarcloud.io/dashboard?id=apache_incubator-ratis

I will create an action plan / umbrella Jira to fix all of these and we should be in a clean state soon.

@elek
Copy link
Member

elek commented May 28, 2020

I confirmed we are able to see the Sonar analyses : https://sonarcloud.io/dashboard?id=apache_incubator-ratis

Wow. Great work.

symious pushed a commit to symious/ratis that referenced this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants