Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1847713652
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
I explored further on this, have the following possible options:
### Option 1
- Remove version from maven-dependency-plugin, hence it defaults to 3.2.0.
- Set version of maven-pmd-plugin as 3.26.0 (this is the latest version).
- Remove `<rule
ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>` from
[pmd-ruleset.xml](https://github.com/apache/druid/blob/master/codestyle/pmd-ruleset.xml)
(because there were hundreds of errors)
- With the above changes, we get a LOT of warnings (hence the check fails)
like:
```
[WARNING] Non-test scoped test only dependencies found:
[WARNING] org.apache.zookeeper:zookeeper-jute:jar:3.8.4:compile
[WARNING] commons-codec:commons-codec:jar:1.16.1:compile
[WARNING] joda-time:joda-time:jar:2.12.5:compile
[WARNING] org.apache.curator:curator-client:jar:5.5.0:compile
```
To address the above, I added `<scope>provided</scope>` to the dependencies
mentioned in the above warning message. I did that in 5 modules, but since this
would have to be done manually for (most likely) almost all 80 modules, I
thought we should discuss first if we want to go with this approach.
### Option 2
- Set maven-dependency-plugin version to 3.3.0. This allows us to add a
configuration to pom.xml to ignore these type of errors, so add the following:
```
<configuration>
<ignoredNonTestScopedDependencies>
<ignoredNonTestScopedDependency>*</ignoredNonTestScopedDependency>
</ignoredNonTestScopedDependencies>
</configuration>
```
- Set version of maven-pmd-plugin as 3.26.0 (this is the latest version).
- Remove `<rule
ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>` from
[pmd-ruleset.xml](https://github.com/apache/druid/blob/master/codestyle/pmd-ruleset.xml)
(because there were hundreds of errors)
### Option 3
- Keep the existing PR as it is. This means:
- maven-dependency-plugin == 3.1.2
- maven-pmd-plugin == 3.16.0
- Once this PR is merged, have a follow-up work item, where we proceed with
either option 1 or option 2. In that follow-up PR, we can also work on the
`UnnecessaryFullyQualifiedName` violations, instead of removing this from
`pmd-ruleset.xml`
<hr>
My thoughts: Option 1 would affect a lot of files, and make it difficult to
review the current PR. So I feel that we should either go with Option 2 in this
PR, or go with Option 3.
What do you think? Would appreciate your inputs on the above, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
