Run checkstyle as separate CI job on Linux with latest Java #584
Conversation
Summary of ChangesHello @horgh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the project's Maven configuration to decouple Checkstyle execution from the main test phase. By moving Checkstyle into a dedicated plugin configuration and removing its profile-based execution, it enables running Checkstyle as an independent CI job on Linux with a modern Java version (Java 24). This change aims to improve CI efficiency by preventing Checkstyle from running on Windows/macOS test builds and enhances maintainability by allowing automated updates of the Checkstyle version via Dependabot. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly moves the Checkstyle execution out of the test phase, setting it up for a dedicated CI job. This change will help speed up local builds on platforms where the check is not needed. I've added one comment regarding a potential inconsistency this change introduces with the Checkstyle version used for Maven site reporting. Adopting Maven best practices by centralizing the plugin configuration using <pluginManagement> would resolve this and improve long-term maintainability.
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-checkstyle-plugin</artifactId> | ||
| <version>3.6.0</version> | ||
| <configuration> | ||
| <consoleOutput>true</consoleOutput> | ||
| <configLocation>checkstyle.xml</configLocation> | ||
| <violationSeverity>warning</violationSeverity> | ||
| </configuration> | ||
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.puppycrawl.tools</groupId> | ||
| <artifactId>checkstyle</artifactId> | ||
| <version>13.0.0</version> | ||
| </dependency> | ||
| </dependencies> | ||
| </plugin> |
There was a problem hiding this comment.
This plugin definition correctly configures Checkstyle for CI runs. However, it creates an inconsistency with the maven-checkstyle-plugin configuration used for reporting (mvn site) on lines 278-292. The reporting plugin will use a different (older) version of Checkstyle because it doesn't have this explicit dependency override. This can lead to different validation results between the CI check and the generated site report.
To ensure consistency and follow Maven best practices, I recommend managing common plugin configurations in a <pluginManagement> section. This would allow both the build and reporting executions to share the same version and dependencies, preventing discrepancies and improving maintainability.
horgh
force-pushed
the
wstorey/checkstyle-separate-job
branch
2 times, most recently
from
51eae60 to
13fe653
Compare
The reporting section for checkstyle is not used - mvn site is never run in CI or documented in release process. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move checkstyle out of the test phase and into a dedicated CI job. This allows dependabot to update the checkstyle version and speeds up test runs on Windows/macOS by not running checkstyle there. Checkstyle 13+ requires Java 21+, so we run it on Java 24 on Linux. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move checkstyle out of the test phase and into a dedicated CI job. This allows dependabot to update the checkstyle version and speeds up test runs on Windows/macOS by not running checkstyle there.
Checkstyle 13+ requires Java 21+, so we run it on Java 24 on Linux.