nzw921rx commented on PR #11791: URL: https://github.com/apache/seatunnel/pull/11791#issuecomment-5378961290
> Reviewed the latest head [eb566e5](https://github.com/apache/seatunnel/commit/eb566e5c8668315d9b276a9966a88188eb228e09). The changes look good overall — found one issue worth confirming before merge: > > **Issue 1(MAJOR)**: Direct-module trigger runs with both RUN_ALL_CONTAINER and RUN_ZETA_CONTAINER as 'false' > > Location: .github/workflows/backend.yml — iceberg-connector-it (L1165) / hbase-connector-it (L1194) > > Description: The old all-connectors-it-8 was gated by api == 'true' || engine == 'true', so at least one of RUN_ALL_CONTAINER / RUN_ZETA_CONTAINER was always 'true'. The new jobs add a third trigger — contains(it-modules, 'connector-iceberg-e2e') — while copying the env block verbatim. A PR that only touches the Iceberg/HBase connector will fire the job with both flags as 'false', a combination these suites never ran with before. If the E2E framework gates container startup on these flags, direct connector changes would run with reduced coverage. > > Suggestion: Verify how the E2E framework handles double-false. If containers are skipped, consider deriving the flag from the full trigger condition (e.g. RUN_ALL_CONTAINER: ${{ needs.changes.outputs.api == 'true' || contains(needs.changes.outputs.it-modules, 'connector-iceberg-e2e') }}). If this is already a known-safe pattern shared by the other 12+ dedicated connector jobs from #11291, a brief comment noting it is intentional would help. Thank you for your review suggestion. I have confirmed it locally😄 <img width="2940" height="1632" alt="image" src="https://github.com/user-attachments/assets/278da1fc-9063-4ccc-aeb9-0e3c18b144e7"; /> 1. I manually set it to false locally and did not find any skipping situations 2. Splitting is due to the long running time of these two combinations, in order to flatten the average time consumption and improve the running speed of CI 3. Modify and start one separately, and another should not run. If there is B in dependency A in mvn parsing, B will still run -- 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]
