[karthik-0306]

Purpose of this pull request

Related to #12707.

No GitHub workflow runs the tests under tools/seatunnel-python-sdk today. The only Python workflow, cli.yml, is limited to seatunnel-cli/**. As a result, the existing test_sdk.py and the new test_java_detector.py (added in #12740) only run when someone runs them locally. This was pointed out during the review of #12740, and @SEZ9 agreed on #12707 that it should be a separate, small PR.

This PR adds .github/workflows/python-sdk.yml, modeled on cli.yml:

• Runs only when tools/seatunnel-python-sdk/** or the workflow file itself changes, on pull requests and on pushes to dev.
• Runs pytest on Python 3.9 (the python_requires floor in setup.py) and 3.13 (a current release).
• Steps: pip install -e . pytest, then python -m pytest tests -v.
• Read-only contents permission, a concurrency group that cancels superseded runs, and a 15-minute timeout.

No lint job is added, because the SDK has no ruff configuration yet. Adding one is out of scope here.

Does this PR introduce any user-facing change?

No. This is a CI-only change. No SDK code, engine code, configuration, or documentation is modified.

How was this patch tested?

• Ran the workflow's steps locally in a fresh virtualenv from tools/seatunnel-python-sdk/:

  python -m pip install -e . pytest
  python -m pytest tests -v

[DanielLeens]

Thanks for adding focused coverage for the Python SDK. I reviewed the workflow against tools/seatunnel-python-sdk/setup.py, the existing test layout, and the repository's CI trigger boundaries.

What Problem Does This PR Solve?

• User pain point: SDK tests could regress without a pull-request or dev-push gate.
• Fix approach: add a path-filtered workflow that installs the SDK and runs pytest on the declared minimum Python and a current Python release.
• One-sentence summary: this PR gives the Python SDK an isolated, least-privilege regression gate.

1. Code Change Review

1.1 Core Logic Analysis

Before this change, edits under tools/seatunnel-python-sdk/ were not selected by the existing CLI workflow. After this change, a matching pull request or dev push triggers the new workflow:

SDK/workflow path changes
  -> python-sdk.yml pull_request or push trigger
  -> actions/checkout + actions/setup-python
  -> pip install -e . pytest
  -> python -m pytest tests -v

The normal path reaches the job. The filters cover both SDK files and workflow edits, the working directory matches the package root, and setup.py declares Python >=3.9, matching the lower matrix entry. The workflow has read-only contents permission, bounded execution time, and concurrency cancellation. No blocking issues found.

1.2 Compatibility Impact

Fully compatible. No product API, configuration, protocol, serialization format, or runtime default changes.

1.3 Performance / Side-Effect Analysis

The workflow runs only for relevant paths. The two-version matrix, pip cache, 15-minute timeout, and cancellation group bound CI cost. No engine/runtime resource impact exists.

1.4 Error Handling and Logging

No formal issues. Pytest output is retained in the workflow log, and the job does not suppress failures.

2. Code Quality Assessment

2.1 Coding Standards

The workflow follows the repository's action pinning style, uses minimal permissions, and documents the Python-version rationale and no-engine test boundary.

2.2 Test Coverage and Test Stability

The job runs the full existing SDK suite on Python 3.9 and 3.13. Stability rating: Stable. The suite uses mocked HTTP and Java detection and has no fixed port or service-container dependency. The current Build check is green.

Local build/test: not executed; this CI-only review was completed from the fetched workflow diff, package metadata, test layout, and current check results.

2.3 Documentation Updates

No user documentation update is required for a CI-only change.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The workflow is separate from the CLI workflow, so its triggers and failures remain attributable to the SDK.

3.2 Maintainability

One small workflow with explicit paths, working directory, and version matrix is easy to update alongside the package.

3.3 Extensibility

Additional supported versions or linting can be added to this dedicated workflow without broadening unrelated CI jobs.

3.4 Historical-Version Compatibility

Fully compatible; no upgrade action is required.

4. Issue Summary

No formal issues were found.

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers — must be fixed

   • None.

2. Recommended fixes — non-blocking

   • None.

Overall assessment: this is a focused, well-scoped CI addition. Keeping the SDK in its own workflow is the better alternative to overloading the CLI workflow.

[DanielLeens]

The CI fact cited in my previous review has changed: Build for the current unchanged head is now passing. CI was previously the only remaining blocker; CI is now passing, so my technical conclusion changes to approve. I do not see a new source-side blocker from Daniel's review. This branch is behind the latest dev base, which I have recorded as merge metadata only; because the current head CI is green, that fact alone does not call for an additional source change. I have read-only review controls, so this COMMENT does not change GitHub's displayed review decision; a write-capable maintainer must perform any final approval or dismissal action.

[davidzollo]

+1
LGTM

Thanks for your contribution! This is a solid addition to the project, and your attention to detail really shows. Contributors like you are what keep this community moving forward. Feel free to keep the PRs coming — we're always glad to have you here!

Please ignore the following message if you have joined in the Slack~

To facilitate focused communication among contributors, we’ve created a brand-new dedicated #contributors channel in official Slack workspace.
Slack workspace link: https://s.apache.org/seatunnel-slack

This channel is designed for technical discussions, feature design brainstorming, roadmap sync and direct communication with project core Contributors. Everyone who contributes code, docs, test cases or community ideas is warmly invited to join, share thoughts and collaborate together under The Apache Way. Welcome to join 😄

[karthik-0306]

Thanks @davidzollo for the approval! I noticed the merge queue bot removed this PR "due to no response for status checks", while Build is green on the current head. Could you re-add it to the merge queue when you have a moment? Happy to rebase on the latest dev if that helps.

[DanielLeens]

Thanks for the note, @karthik-0306. For reference, the current head 99c1b43a has a passing Build and an approval from @davidzollo, and my own review has no open source-side blocker.

I am a read-only reviewer here, so I cannot re-add the PR to the merge queue myself. @davidzollo, could you re-enqueue it when you have a moment, since the bot removed it for a status-check timeout rather than a failure?

On the rebase offer: the branch is 24 commits behind dev, but nothing in the PR-owned files depends on those commits, and the merge queue builds its own merge commit against the latest base. A rebase is therefore not required for correctness. If the queue rejects the entry again, syncing with dev is a cheap way to give it a fresh run.

[DanielLeens]

This is a CI re-check on the unchanged head 99c1b43ae. In my previous review on this head, CI was the only remaining blocker and I had no source-side finding (conclusion was Ready to merge for the Python SDK pytest workflow). CI on this head is now passing: the apache-side Build check (completed successfully) points to a successful fork run for this exact SHA. Since CI was the only remaining blocker and it is now green, my conclusion changes to approve.

Status of the rest: the head has not changed, there is no new author or reviewer feedback that changes the technical picture, and @davidzollo has already approved. The merge queue removed the PR for a status-check timeout rather than a failure, so it only needs to be re-enqueued by a maintainer; a rebase on dev is not required for correctness.

As a read-only reviewer I cannot merge or satisfy branch protection myself, so a maintainer with write access still needs to do the final approval and merge. Thanks again for the contribution!

[davidzollo]

基本信息

• 标题：[Improve][CI] Run the seatunnel-python-sdk test suite in CI
• 中文释义：为 tools/seatunnel-python-sdk 增加独立的 GitHub Actions 测试工作流
• 作者：karthik-0306
• 目标分支：dev
• 变更量：+74 / -0，1 个文件
• 状态：OPEN，可合并
• Review 报告时间：2026-10-21 09:20:53 EDT (UTC-04:00)
• 本地分支：pr-12857（head 99c1b43ae840）
• PR 链接：#12857

PR 解决了什么问题（小白示例）

• 用户痛点：tools/seatunnel-python-sdk 下已有 test_sdk.py 和 #12740 新增的 test_java_detector.py，但没有任何工作流运行它们；只有 cli.yml 跑 seatunnel-cli。改坏 SDK 的 PR 在 CI 上看不出来。
• 修复方式：新增 .github/workflows/python-sdk.yml，仿照 cli.yml：只在 SDK 目录或工作流自身变化时触发，在 Python 3.9 和 3.13 上 pip install -e . pytest 后运行 pytest tests -v。
• 一句话：SDK 的现有测试从“只能本地手动跑”变成“每个相关 PR 自动跑”。

一、代码变更审查

1.1 核心逻辑分析

变更内容精确描述

新文件 python-sdk.yml，关键声明：

on:
  push: { branches: [dev], paths: ['tools/seatunnel-python-sdk/**', '.github/workflows/python-sdk.yml'] }
  pull_request: { paths: [同上] }
permissions:
  contents: read
concurrency:
  group: python-sdk-${{ github.event.pull_request.number || github.ref }}
  cancel-in-progress: true
defaults: { run: { working-directory: tools/seatunnel-python-sdk } }
jobs.test: ubuntu-latest, timeout-minutes 15, matrix python-version ['3.9', '3.13'], fail-fast false
steps: checkout@v4 -> setup-python@v5 (cache pip, cache-dependency-path tools/seatunnel-python-sdk/setup.py) -> pip install -e . pytest -> python -m pytest tests -v

关键发现

• 这是 CI 工作流 PR，必须按权限面审查：permissions: contents: read 是最小权限；触发器只有 push(dev) 和 pull_request，没有 pull_request_target / workflow_run，不使用任何 secret；pull_request 对 fork 使用只读 token。安全面没有问题。
• 不是必需检查（分支保护不依赖它），且带 paths 过滤，不会增加无关 PR 的 CI 负担。
• 3.9 对应 setup.py 的 python_requires 下限，3.13 是当前版本，矩阵选择有依据。
• 精准的新增能力，不改任何现有工作流。

逻辑正确性深度分析

• defaults.run.working-directory 影响 run 步骤，而 setup-python 的 cache-dependency-path 相对仓库根目录解析，PR 写的是完整路径 tools/seatunnel-python-sdk/setup.py，两者一致。
• 作者说明测试用 mock 替代 HTTP 和 java -version，因此不需要 JDK、引擎或网络；DanielLeens 的评论确认 fork 侧该工作流已成功运行。
• concurrency 用 PR 号或 ref 分组并取消过期运行，与 cli.yml 一致。

1.2 兼容性影响

完全兼容。不改 SDK、引擎、配置或文档。

1.3 性能 / 副作用分析

每个相关 PR 增加两个约 1 分钟的 Python 任务；paths 过滤限制触发范围；无其它副作用。

1.4 错误处理与日志

fail-fast: false 让两个版本独立报告；pytest -v 输出足够定位失败用例。

1.5 引擎改动专项检查

不适用。

二、代码质量评估

2.1 代码规范

ASF 许可证头齐全；注释解释了矩阵选择和为何不需要 JDK；action 版本与仓库其它工作流一致。

2.2 测试覆盖

本 PR 本身是测试基础设施。它让 SDK 现有测试进入 CI；未新增 lint，作者已说明 SDK 尚无 ruff 配置，属于合理的范围控制。

本地验证结果

• git fetch upstream pull/12857/head:pr-12857：通过；head 99c1b43ae840。
• git diff upstream/dev...pr-12857：通过；逐行阅读 74 行新工作流。
• gh pr view 12857 --json ... / gh pr checks 12857：通过；Build SUCCESS；merge state 为 UNSTABLE，由非必需检查状态导致，已有 davidzollo 和 DanielLeens 的 APPROVE。
• 编译 / 测试：未执行。原因：Review PR 默认不做本地编译；YAML 已在 fork CI 上实际运行。

2.3 文档更新

不需要。

三、架构合理性

3.1 解决方案的优雅性

最佳改动位置评估

• 当前改动位置：.github/workflows/python-sdk.yml，独立工作流。
• 根因位置：根因不适用（新增能力）；职责归属是“SDK 的测试应由 SDK 自己的工作流承载”。
• 是否为最佳改动：是。把 SDK 测试塞进 cli.yml 或 backend.yml 会让无关 PR 触发，也混淆了两个 Python 项目的职责。
• 全局最佳改动位置：同上。
• 最小安全改动与全局最佳改动：一致。
• Merge 影响：精准新增，不阻塞。

3.2 可维护性

与 cli.yml 同构，后续维护者一看即懂。

3.3 扩展性

后续加 lint 只需增加一个 job。

3.4 历史版本兼容性

不涉及。

四、问题汇总

全量问题穷尽自检

• PR diff / changed files：1 个文件已逐行审查完。
• 关键调用链：触发器 -> 权限 -> 并发 -> 步骤，已核对；权限面为最小。
• 根因 / 当前改动位置 / 最佳改动位置 / 方案取舍：已判断。
• 兼容性 / 升级路径 / 协议或配置契约：不涉及。
• 测试分层 / 文档 / 示例 / 默认配置：已核对。
• Whale 协同仓库：不涉及。
• CI / Review / 冲突 gate：Build SUCCESS，已有两位 reviewer APPROVE，无冲突。
• 无法审查范围：无。

未发现阻塞性问题。剩余风险：无；后续若 SDK 增加对外网络依赖，需要同步调整该工作流。

五、是否可以 Merge 的结论

结论：可以 merge

阻塞项（必须修复）：无。
建议修复（非阻塞）：无。
整体评价：最小权限、路径过滤、与现有 cli.yml 同构的 CI 新增，补上了 SDK 测试长期无人运行的缺口。