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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org