DanielLeens commented on PR #12845: URL: https://github.com/apache/seatunnel/pull/12845#issuecomment-6289512600
Correction to my earlier review of head `577037b5d`, which I want to make before anyone relies on it. In the review I wrote that workflow changes route to the full suite and that the `engine-k8s-it` lane on this PR would be the first real proof of the pinned k3s version. That was wrong. In `.github/workflows/backend.yml` the job `engine-k8s-it` only runs when `changes.outputs.api == 'true'`, `changes.outputs.engine == 'true'`, or `it-modules` contains `seatunnel-engine-k8s-e2e`. A workflow-only diff matches none of these, and in the fork run for this SHA the job is indeed reported as skipped (together with `unit-test` and the updated-modules shards). The `Build` check is green, but it does not execute the changed install step, so it is not evidence for the pin, the retry loop or the checkout reordering. What I can still say from reading the diff: the pin, the `pipefail` retry and the checkout-before-install ordering are correct as described in my review, and I have no code-side blocker. The remaining risk is that the first execution of the new k3s path (with `v1.36.5+k3s1` and `--docker`) will happen after merge, on the next PR that touches engine code. If you can, a manual `workflow_dispatch`-style run or a throwaway commit that touches an engine file on your fork would give real evidence before merge; otherwise a maintainer needs to accept that residual risk. My two Low suggestions (Helm job `pipefail`/retry, a bump note for the pinned version) are unchanged and non-blocking. -- 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]
