Re-enable the Spotless plugin for Java 21 #12992
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12992 +/- ##
============================================
+ Coverage 61.75% 62.15% +0.40%
+ Complexity 207 198 -9
============================================
Files 2436 2502 +66
Lines 133233 136586 +3353
Branches 20636 21145 +509
============================================
+ Hits 82274 84895 +2621
- Misses 44911 45398 +487
- Partials 6048 6293 +245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
yashmayya
force-pushed
the
reenable-spotless-java-21
branch
from
e49f3d1
to
ef86863
Compare
| <plugin> | ||
| <groupId>com.diffplug.spotless</groupId> | ||
| <artifactId>spotless-maven-plugin</artifactId> | ||
| <configuration> |
There was a problem hiding this comment.
We usually keep the plugin management inside the root pom, and then we can simply use it here
There was a problem hiding this comment.
I think this change is fine. Basically he moved this from pluginManagement to plugin. Given there is no child project that inherits its pom, I think this is the right move.
There was a problem hiding this comment.
Yeah, I assumed that #11670 introduced the pluginManagement in this POM for the JDK 21 build profile (i.e., to avoid running this plugin's goal with Java 21)? Also we're overriding the plugin configuration here (for the separate excludes I presume), so it's not like we can simply define it here without the configuration right?
Edit: We're still leveraging the plugin management from the parent POM since we aren't redefining the plugin version or executions here.
There was a problem hiding this comment.
What I meant is that we should be able to put the excludes in the root pom where there is another configuration for this plug-in, then we keep a single configuration for this plug-in for easier management
There was a problem hiding this comment.
Ah okay, I've made that change. Not sure why it was like that earlier - perhaps to avoid adding child module specific excludes in the parent POM? Looks like it was added in https://github.com/apache/pinot/pull/8427/files.
There was a problem hiding this comment.
What I meant is that we should be able to put the excludes in the root pom where there is another configuration for this plug-in, then we keep a single configuration for this plug-in for easier management
I think that is a bad practice. This is the project that knows which files should be ignored. In case other projects want to do the same we don't want to merge that information.
The pom project should specify the plugin version and the common configuration. Configuration specific to each sub-project should be configured by each sub-project
There was a problem hiding this comment.
Makes sense, I've reverted the last commit to keep things as they were previously.
There was a problem hiding this comment.
@gortiz Does it work if we only keep the excludes config here? Keeping separate excludes per module definitely make sense, but replicating other fields can add management overhead
There was a problem hiding this comment.
Based on https://maven.apache.org/pom.html#plugins, that should work and it is supported behavior. I've pushed a commit making this change and also verified locally that it works as expected.
| @@ -1625,7 +1611,6 @@ | |||
| <version>2.43.0</version> | |||
| <executions> | |||
| <execution> | |||
| <phase>verify</phase> | |||
There was a problem hiding this comment.
This isn't required because spotless:check is already bound to Maven's verify phase by default (see https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#binding-to-maven-phase).
| <plugin> | ||
| <groupId>com.diffplug.spotless</groupId> | ||
| <artifactId>spotless-maven-plugin</artifactId> | ||
| <configuration> |
There was a problem hiding this comment.
Ah okay, I've made that change. Not sure why it was like that earlier - perhaps to avoid adding child module specific excludes in the parent POM? Looks like it was added in https://github.com/apache/pinot/pull/8427/files.
…ot-common POM" This reverts commit 2542ea0.
| <plugin> | ||
| <groupId>com.diffplug.spotless</groupId> | ||
| <artifactId>spotless-maven-plugin</artifactId> | ||
| <configuration> |
There was a problem hiding this comment.
@gortiz Does it work if we only keep the excludes config here? Keeping separate excludes per module definitely make sense, but replicating other fields can add management overhead
…that are duplicated from parent POM
2.41.1of thespotless-maven-plugin.2.28.0to2.43.0and it can now be re-enabled for Java 21.mvn spotless:checkandmvn spotless:applywhile using Java 21.