Skip to content

HDDS-9885. Checkstyle check passing despite config error#5755

Merged
kerneltime merged 1 commit intoapache:masterfrom
adoroszlai:HDDS-9885
Dec 20, 2023
Merged

HDDS-9885. Checkstyle check passing despite config error#5755
kerneltime merged 1 commit intoapache:masterfrom
adoroszlai:HDDS-9885

Conversation

@adoroszlai
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

basic (checkstyle) is shown as passing if it fails with config error, not a checkstyle violation.

https://issues.apache.org/jira/browse/HDDS-9885

How was this patch tested?

Verified that the check fails with 9b16de3 checked out (which had the config error):

$ hadoop-ozone/dev-support/checks/checkstyle.sh > /dev/null

$ hadoop-ozone/dev-support/checks/_summary.sh target/checkstyle/summary.txt
13:58:11,624 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check (default-cli) on project ozone-main: Failed during checkstyle configuration: cannot initialize module TreeWalker - TreeWalker is not allowed as a parent of LineLength Please review 'Parent Module' section for this Check in web documentation if Check is standard. -> [Help 1]

$ echo $?
1

Verified the check still fails if there is checkstyle violation:

$ hadoop-ozone/dev-support/checks/checkstyle.sh > /dev/null

$ hadoop-ozone/dev-support/checks/_summary.sh target/checkstyle/summary.txt
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
 91: Line is longer than 80 characters (found 82).
 92: Line is longer than 80 characters (found 81).

$ echo $?
1

Verified the check still passes with code conforming to checkstyle rules:

$ hadoop-ozone/dev-support/checks/checkstyle.sh > /dev/null

$ hadoop-ozone/dev-support/checks/_summary.sh target/checkstyle/summary.txt

$ echo $?
0

CI:
https://github.com/adoroszlai/ozone/actions/runs/7158011023/job/19489560474

@adoroszlai adoroszlai self-assigned this Dec 10, 2023
mv output.log "${REPORT_DIR}"/
fi

cat "${REPORT_DIR}/output.log"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not inside the if condition?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Previously it was in the else branch, not the if (as only the first mvn execution was redirected to output.log). With this change, both executions are redirected, so we'd need to cat in both if and else. Which can be simplified by moving this common step after the if.

@adoroszlai adoroszlai added the CI label Dec 14, 2023
@kerneltime kerneltime merged commit fdf8b6a into apache:master Dec 20, 2023
@adoroszlai adoroszlai deleted the HDDS-9885 branch December 20, 2023 07:40
@adoroszlai
Copy link
Copy Markdown
Contributor Author

Thanks @kerneltime for reviewing and merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants