XComp commented on code in PR #136:
URL: https://github.com/apache/flink-shaded/pull/136#discussion_r1467749847
##########
pom.xml:
##########
@@ -65,6 +65,7 @@ under the License.
<netty.version>4.1.100.Final</netty.version>
<jackson.version>2.15.3</jackson.version>
<guava.version>32.1.3-jre</guava.version>
+ <flink.markBundledAsOptional>true</flink.markBundledAsOptional>
Review Comment:
I see, those were different issue. But that still leaves the question open
whether we want to enable the more recent shade plugin version for all modules.
:thinking:
##########
.github/workflows/ci.yml:
##########
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
- -Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+ -Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+ run: |
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
Sorry for being not clear on that one. I didn't mean to move it from the
`Build` step into the `Check shade optional dependencies`. Instead, you could
add a new step `Create Dependency Tree`.
##########
flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml:
##########
@@ -128,6 +131,7 @@ under the License.
<groupId>org.apache.zookeeper</groupId>
<artifactId>zookeeper</artifactId>
<version>${zookeeper.version}</version>
+ <optional>${flink.markBundledAsOptional}</optional>
</dependency>
<dependency>
<groupId>io.netty</groupId>
Review Comment:
Can you elaborate a bit here? It doesn't make sense because dependencies
from the `<dependencyManagement/>` section would need to be explicitly added to
a module pom to make the dependency available and only then would we have to
add the `<optional/>` tag?!
Looks like we have 4 locations in `flink-dist/pom.xml` and
`flink-filesystems/flink-s3-fs-presto/pom.xml` where this also applies.
:thinking:
##########
.github/workflows/ci.yml:
##########
@@ -51,6 +52,14 @@ jobs:
- name: Check licensing
run: |
- mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \
+ mvn ${MVN_COMMON_OPTIONS} exec:java@check-license -N \
-Dexec.args="${{ env.MVN_BUILD_OUTPUT_FILE }} $(pwd) ${{
env.MVN_VALIDATION_DIR }}" \
- -Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
\ No newline at end of file
+ -Dlog4j.configurationFile=file://$(pwd)/tools/ci/log4j.properties
+
+ - name: Check shade optional dependencies
+ run: |
+ mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee
${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
Review Comment:
```suggestion
mvn ${MVN_COMMON_OPTIONS} dependency:tree | tee ${{
env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
```
The profile is not necessary for the `dependency:tree` IIUC.
--
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]
