-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feature/356/add checkstyle GitHub action to Java-SDK pipeline #142
Feature/356/add checkstyle GitHub action to Java-SDK pipeline #142
Conversation
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
b61f839
to
f55c53b
Compare
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
@charankamarapu Can you please review the PR ? |
Will review it by tomorrow 12 pm. |
@charankamarapu |
pom.xml
Outdated
<artifactId>checkstyle</artifactId> | ||
<version>8.31</version> | ||
</dependency> | ||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Java SDK added over here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this runs checkstyle check during release
but including it in build only would be enough I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use profiles to change default build config . As you have already added plugins in . I don't think there is a need to mention them again in profile . Please check this once and remove plugin from profile
checkstyle-config.xml
Outdated
<!-- </module>--> | ||
<!-- <module name="BeforeExecutionExclusionFileFilter">--> | ||
<!-- <property name="fileNamePattern" value="/agent/"/>--> | ||
<!-- </module>--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the checks like - AvoidStarImport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charankamarapu
I avoided to add rules right now to avoid pipeline failure
when I tried to run check it resulted +15K rule violation which won't be practical to solve in one issue and we don't have unit tests to to verify that everything still works as expected after such significant changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I get it but u can use BeforeExecutionExclusionFileFilter - exclude all files and add all the rules so that there will be no errors. Then other developers will see the rules include each module and work on it . I mean If we don't specify rules what will we be benchmarking our code against? Please add the rules and exclude all the modules/files and also add a screenshot by including one module/file which shows all the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure on it
.github/workflows/maven.yml
Outdated
- uses: actions/checkout@v2 | ||
- uses: dbelyaev/action-checkstyle@v0.6.1 | ||
with: | ||
github_token: ${{ secrets.github_token }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is github_token mandatory over here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only to catch info for PR but it's not mandatory even when I deleted this line things still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think that is required . Remove it if it is not necessary.
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
Signed-off-by: We'am Bassem <58189568+We2Am-BaSsem@users.noreply.github.com>
@charankamarapu |
Ship it! |
Related Issue
I chose checkstyle as it's most popular tool for checking java code and it's very easy and usable to be configured
Closes: #356
Describe the changes you've made
I added a the checkstyle plugin in
pom.xml
and configured it to use a specific configuration file I added so we can add and configure a proper set of rules (or we can use a common configuration file such as sun_check.xml).I added an action in the pipeline as well to check the code using the configuration file added by me too so the same rules check applied both on the pipeline action or locally.
Type of change
How did you test your code changes?
I tried those changes on a forked repo and tried to make a PR with the empty configuration file to check it should pass as there are no rules to check then added a rule that should fail the pipeline and pushed it and checked that the pipeline did fail.
when tried to apply rules on agent module this is what I got:
and pipeline failed