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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 24, 2023
Merged

Feature/356/add checkstyle GitHub action to Java-SDK pipeline #142

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
49d40f5
feature/129/add checkstyle plugin + add check style configurration file
We2Am-BaSsem Mar 6, 2023
a41f6e4
feature/129/add checkstyle linting action to the github pipeline
We2Am-BaSsem Mar 6, 2023
c642460
Update checkstyle-config.xml
We2Am-BaSsem Mar 6, 2023
f55c53b
Update maven.yml
We2Am-BaSsem Mar 6, 2023
129c8d2
feature/129/clear rules from configuration file
We2Am-BaSsem Mar 6, 2023
dd646a6
feature/129/setup checkstyle that it fails the pipelin on errors
We2Am-BaSsem Mar 6, 2023
5a4de42
feature/129/remove unnecessary changes + add rules
We2Am-BaSsem Mar 8, 2023
0643e55
feature/129/resolve configuration file
We2Am-BaSsem Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions .github/workflows/maven.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# This workflow will build a Java project with Maven, and cache/restore any dependencies to improve the workflow execution time
# For more information see: https://help.github.com/actions/language-and-framework-guides/building-and-testing-java-with-maven

name: JavaSDK build check
name: JavaSDK build check

on:
push:
Expand All @@ -15,12 +15,25 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
- name: Set up JDK 11
uses: actions/setup-java@v3
with:
java-version: '11'
distribution: 'temurin'
cache: maven
- name: Build with Maven
run: mvn -B package --file pom.xml
- uses: actions/checkout@v3
- name: Set up JDK 11
uses: actions/setup-java@v3
with:
java-version: '11'
distribution: 'temurin'
cache: maven
- name: Build with Maven
run: mvn -B package --file pom.xml

checkstyle:
name: checkstyle linting
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: dbelyaev/action-checkstyle@v0.6.1
with:
github_token: ${{ secrets.github_token }}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

reporter: github-pr-review
checkstyle_config: checkstyle-config.xml
fail_on_error: true

38 changes: 38 additions & 0 deletions checkstyle-config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
<property name="severity" value="error"/>

<property name="fileExtensions" value="java, properties, xml"/>


<!-- This for ignoring specific module when start applying rules so cleaning rules violations would be easier and organized -->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/models/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/core/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/common/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/integration/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/Agent/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/keploy-sdk/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/api/"/>-->
<!-- </module>-->
<!-- <module name="BeforeExecutionExclusionFileFilter">-->
<!-- <property name="fileNamePattern" value="/agent/"/>-->
<!-- </module>-->
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure on it


</module>
37 changes: 35 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="http://maven.apache.org/POM/4.0.0"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

Expand Down Expand Up @@ -66,6 +66,25 @@
<id>release</id>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.31</version>
</dependency>
<dependency>
Copy link
Member

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 ?

Copy link
Contributor Author

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.

Copy link
Member

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

<groupId>io.keploy</groupId>
<artifactId>java-sdk</artifactId>
<version>1.0.0-SNAPSHOT</version>
</dependency>
</dependencies>
<configuration>
<configLocation>checkstyle-config.xml</configLocation>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-source-plugin</artifactId>
Expand Down Expand Up @@ -189,6 +208,20 @@

<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.31</version>
</dependency>
</dependencies>
<configuration>
<configLocation>checkstyle-config.xml</configLocation>
</configuration>
</plugin>
<!-- <plugin>-->
<!-- <groupId>org.apache.maven.plugins</groupId>-->
<!-- <artifactId>maven-gpg-plugin</artifactId>-->
Expand Down