DanielLeens commented on code in PR #12309:
URL: https://github.com/apache/seatunnel/pull/12309#discussion_r4010768477
##########
seatunnel-connectors-v2/connector-file/connector-file-base/pom.xml:
##########
@@ -122,6 +122,11 @@
<version>${commons.lang3.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-secure-xml</artifactId>
Review Comment:
Thanks for pushing back with real evidence here, @ppkarwasz — you're right,
and I want to correct the record rather than let this sit.
I re-traced this from the actual build wiring instead of re-asserting the
earlier claim. `tools/dependencies/checkLicense.sh:26` invokes
`dependency:copy-dependencies` as an ad-hoc CLI goal across the whole reactor
(`-pl '!seatunnel-dist'`), not through a bound lifecycle execution.
`seatunnel-connectors-v2/pom.xml:139-141` — inherited by `connector-file-base`
through `connector-file` → `seatunnel-connectors-v2`, with no override anywhere
in that chain (I checked both intermediate poms) — declares
`maven-dependency-plugin`'s
`<configuration><skip>${e2e.dependency.skip}</skip></configuration>` directly
under `<build><plugins>`, with no `<executions>` block of its own. That's
exactly the "default configuration applied to any direct goal invocation"
pattern, and `${e2e.dependency.skip}` defaults to `true` in the root
`pom.xml:179`. `seatunnel-e2e/pom.xml:82-84` carries the identical
`<skip>${e2e.dependency.skip}</skip>` wiring, so `seatunnel-e2e-common` is
skipped the same way.
So when `checkLicense.sh` runs its ad-hoc `copy-dependencies` invocation,
connector and e2e module dependencies — including the new `commons-secure-xml`
runtime dependency on `connector-file-base` — are never copied into
`/tmp/seatunnel-dependencies`, never make it into
`third-party-dependencies.txt`, and the final `diff` against
`known-dependencies.txt` (`checkLicense.sh:45`) stays clean. You're also right
that proactively adding the jar to `known-dependencies.txt` would flip this
from a non-issue into an actual failure, since that diff is symmetric — it
flags a listed-but-not-found entry just as readily as an unlisted-but-found one.
So I'm withdrawing Issue 1 as a Blocker. Both my own conclusion and my
endorsement of @goutamadwant's comment were wrong on the specific "will fail
`checkLicense.sh`" mechanism — I checked the three inventory files for a
matching entry but never traced the plugin-skip wiring that actually determines
what the script sees in the first place, which was the real gap in my review.
On the separate NOTICE/LICENSE-content question, I don't have counter-evidence
against your reading of the ASF licensing guide (Apache-2.0, no sub-components,
nothing NOTICE-worthy) — that's consistent with how this repo already handles
other bundled ASF-licensed components, so I agree no LICENSE/NOTICE change is
needed there either. And agreed that the deeper gap you flagged —
connector-shaded dependencies never entering this check at all, same as dom4j
today — is real but pre-existing and out of scope for this PR.
Updated merge recommendation: with Issue 1 withdrawn, the only item left
from my review is Issue 2 (the `FIXME: translation pending` placeholders in the
zh docs), which I'd already called Minor/non-blocking pending a
Chinese-speaking maintainer's sign-off on your proposed translations. So my
conclusion moves from "Ready to merge after fixes" to **Ready to merge** —
there's no code-side blocker remaining from my side.
Separately, unrelated to this thread: `Build` is still showing
`action_required` on the `apache/seatunnel` side as of this check (head
`3cbdf136925`), and I still see zero Actions runs on your fork for this branch
— same state as my last review. Could you confirm GitHub Actions is enabled on
your fork, or push an empty commit to retrigger, so we get a real CI run before
merge?
--
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]
