Skip to content

ci: validate PDF flush workflow inputs - #22026

Merged
ti-chi-bot[bot] merged 3 commits into
pingcap:masterfrom
lilin90:validate-flush-version-input
Nov 3, 2026
Merged

ti-chi-bot[bot] merged 3 commits into
pingcap:masterfrom
lilin90:validate-flush-version-input

Conversation

@lilin90

@lilin90 lilin90 commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

What changed

  • restrict the privileged PDF flush job to active documentation maintainers
  • disable all GITHUB_TOKEN permissions for the workflow
  • validate TiDB and TiDB Operator versions before using them
  • pass the workflow input to shell through environment variables instead of interpolating it into run: scripts
  • expose Tencent Cloud credentials only to the CDN refresh step
  • add regression tests for malicious inputs, actor restrictions, permissions, and credential scope

Validation

  • python3 scripts/test-flush-version-workflow.py (6 tests passed)
  • parsed .github/workflows/flush-version.yaml successfully with PyYAML
  • git diff --check

Summary by CodeRabbit

  • 改进
    • 增加版本输入校验:非 TiDB Cloud 类型仅接受指定格式的版本值;无效输入会导致工作流失败。
    • 限制版本刷新工作流仅由指定维护者触发,并将腾讯云凭据配置限定在刷新步骤。
  • 测试
    • 增加工作流测试,覆盖权限配置、版本校验及恶意输入处理等场景。

@ti-chi-bot ti-chi-bot Bot added missing-translation-status This PR does not have translation status info. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2026
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 9 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between b94b3f4 and c28537d.

Files selected for processing: 1
  • .github/workflows/flush-version.yaml
Walkthrough

工作流新增触发者限制和版本校验。TiDB 与 TiDB Operator 链接步骤改用步骤环境变量传递版本。腾讯云凭据移至 Refresh URLs 步骤。新增测试覆盖工作流配置、版本输入和凭据作用域。

Changes: 版本刷新工作流控制

Layer / File(s) Summary
运行者限制与版本校验
.github/workflows/flush-version.yaml, scripts/test-flush-version-workflow.py
工作流新增空权限声明、触发者条件和版本校验步骤。测试检查相关配置、有效与无效版本,以及恶意输入。
PDF 链接与凭据作用域
.github/workflows/flush-version.yaml, scripts/test-flush-version-workflow.py
TiDB 和 TiDB Operator 链接步骤通过步骤环境变量读取版本,并用 printf 写入链接。腾讯云凭据仅设置在 Refresh URLs 步骤,测试检查凭据作用域。

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested labels: type/bugfix, size/M

Suggested reviewers: oreoxmt

Merge Risk: Low · previous review b94b3

The workflow blocks manual cache refreshes for supported stable PDFs. This is a narrow operational issue, while the permission-test gap should also be strengthened before relying on the security regression coverage.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning 描述列出了变更内容(执行者限制、GITHUB_TOKEN 权限、版本校验、环境变量传递、腾讯云凭据范围、回归测试)和验证步骤。但描述未包含模板要求的多数章节:未勾选 Contributor License Agreement 检查项,未说明受影响的 TiDB 版本分支(“Which TiDB version(s) do your changes apply to?”,必填),未填写相关 PR 或文… 补充以下内容:1. 在“Which TiDB version(s) do your changes apply to?”部分勾选受影响的版本分支。对于 CI 工作流变更,通常仅勾选 master。2. 在“What is changed, added, or deleted?”部分保留或改写变更说明,使其符合模板章节。3. 在“What is the related PR or file link(s)?”部分补充相关链接(如无则留空并说明)。4. 勾选 Contributo…
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed 标题“ci: validate PDF flush workflow inputs”概括了主要变更:对 PDF 刷新工作流的输入进行校验。变更集还包括权限收紧、执行者限制、凭据范围调整和回归测试,标题未涵盖这些内容,但未涵盖的部分属于次要细节,可接受。标题简洁,与变更集相关。
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

描述列出了变更内容(执行者限制、GITHUB_TOKEN 权限、版本校验、环境变量传递、腾讯云凭据范围、回归测试)和验证步骤。但描述未包含模板要求的多数章节:未勾选 Contributor License Agreement 检查项,未说明受影响的 TiDB 版本分支(“Which TiDB version(s) do your changes apply to?”,必填),未填写相关 PR 或文件链接,未选择 AI agent 参与情况,也未勾选变更类型说明。由于必填的版本选择部分缺失,描述不完整。

Resolution

补充以下内容:1. 在“Which TiDB version(s) do your changes apply to?”部分勾选受影响的版本分支。对于 CI 工作流变更,通常仅勾选 master。2. 在“What is changed, added, or deleted?”部分保留或改写变更说明,使其符合模板章节。3. 在“What is the related PR or file link(s)?”部分补充相关链接(如无则留空并说明)。4. 勾选 Contributor License Agreement 项(如适用)。5. 在“AI agent involvement”和“Do your changes match any of the following descriptions?”部分完成勾选。

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 1 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@lilin90 lilin90 self-assigned this Nov 3, 2026
@lilin90 lilin90 added the translation/no-need No need to translate this PR. label Nov 3, 2026
@ti-chi-bot ti-chi-bot Bot removed the missing-translation-status This PR does not have translation status info. label Nov 3, 2026
@lilin90 lilin90 added the ONCALL Relates to documentation oncall. label Nov 3, 2026
@lilin90
lilin90 requested a review from qiancai November 3, 2026 09:30
Comment thread .github/workflows/flush-version.yaml Resolved

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (2)

Minor · 允许手动刷新 stable PDF。 · flush-version.yaml:42-45

.github/workflows/flush-version.yaml:42-45
Minor · Functional Correctness · Quick win

允许手动刷新 stable PDF。

resources/cdnfresh.txt 将 TiDB 和 TiDB Operator 的 stable PDF 列为规范 URL。当前校验拒绝 stable,工作流会在生成链接前退出,因此不会执行 CDN purge。请将 stable 加入允许的版本值。

建议修复
-          if [[ ! "$VERSION" =~ ^(dev|v[0-9]+\.[0-9]+(\.[0-9]+)?)$ ]]; then
-            echo "Invalid version: expected dev, vMAJOR.MINOR, or vMAJOR.MINOR.PATCH" >&2
+          if [[ ! "$VERSION" =~ ^(dev|stable|v[0-9]+\.[0-9]+(\.[0-9]+)?)$ ]]; then
+            echo "Invalid version: expected dev, stable, vMAJOR.MINOR, or vMAJOR.MINOR.PATCH" >&2
Trivial · 权限测试未检查作业级 permissions。 · test-flush-version-workflow.py:33-43

scripts/test-flush-version-workflow.py:33-43
Trivial · Security & Privacy · Quick win

权限测试未检查作业级 permissions。

test_workflow_has_no_token_permissions 只检查工作流级 permissions(scripts/test-flush-version-workflow.py:33-34)。作业级 permissions 会覆盖工作流级设置。如果 flush 作业添加了 permissions: {contents: write} 等授权,此测试仍会通过。该作业执行特权操作,因此回归会使 GITHUB_TOKEN 获得本 PR 要禁止的权限,而测试不会报告。

建议修复
     def test_workflow_has_no_token_permissions(self):
         self.assertEqual(self.workflow["permissions"], {})
+        self.assertEqual(self.job.get("permissions", {}), {})

该断言接受缺失键和空映射 {}。两者都不授予 GITHUB_TOKEN 权限。

Review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 4e32aa5 and b94b3f4.

Files selected for processing: 2
  • .github/workflows/flush-version.yaml
  • scripts/test-flush-version-workflow.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

@qiancai qiancai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

Comment thread .github/workflows/flush-version.yaml Resolved
@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-11-03 10:06:56.098624677 +0000 UTC m=+574740.723774929: ☑️ agreed by qiancai.

Co-authored-by: Grace Cai <qqzczy@126.com>
@lilin90 lilin90 added the lgtm label Nov 3, 2026

@lilin90 lilin90 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@ti-chi-bot

ti-chi-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilin90

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the approved label Nov 3, 2026
@ti-chi-bot
ti-chi-bot Bot merged commit 425f7f6 into pingcap:master Nov 3, 2026
11 checks passed
@lilin90
lilin90 deleted the validate-flush-version-input branch November 3, 2026 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm needs-1-more-lgtm Indicates a PR needs 1 more LGTM. ONCALL Relates to documentation oncall. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. translation/no-need No need to translate this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants