ci(pingcap-qe/ci): add PR content policy check#4476
Conversation
Update flux-cli image from v2.0.0-rc.4 to v2.2.3 in presubmit jobs for pingcap-qe/ci and ti-community-infra/configs repositories.
Add a new presubmit job `pull-verify-pr-content-policy` that runs the newly added `.ci/check-pr-content-policy.sh` script. The script scans added lines in PRs for policy violations, currently checking for forbidden literal substrings and unauthorized pingcap.net hosts.
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This pull request introduces a new content policy check for PRs, enforces it via a presubmit Prow job, migrates Jenkins URLs to a new domain, and improves container image management and security in CI pipelines. The approach is well-structured, with a clear separation of responsibilities across scripts and configuration updates. The overall quality is good, but there are opportunities for improvement in error handling, testing coverage, and edge case handling in the policy check script.
Critical Issues
-
Error Handling in
check-pr-content-policy.sh(Lines 171-173)- Issue: The script fails silently if
grepdoesn't match any patterns during host validation (grepreturns a non-zero exit code which is ignored). This could lead to missing important violations. - File:
.ci/check-pr-content-policy.sh - Suggestion:
Replace the pipeline with an explicit check to handlegrep's exit code:Add a test case to verify behavior when no matches occur.while IFS= read -r host; do [[ -z "$host" ]] && continue lower_host="${host,,}" if ! is_allowed_pingcap_host "$lower_host"; then append_unique_value "contains forbidden host ${lower_host}" "$reasons_name" fi done < <(grep -ioE "$PINGCAP_HOST_PATTERN" <<<"$content" || true)
- Issue: The script fails silently if
-
Memory Resource Allocation for the New Prow Job
- Issue: The memory limit for the
pull-verify-pr-content-policyjob is set to128Mi, which may be insufficient for larger diffs with many violations. - File:
prow-jobs/pingcap-qe/ci/presubmits.yaml(Line 18) - Suggestion: Increase the memory limit to
256Mito accommodate larger diffs and avoid potential OOM errors during policy checks.
- Issue: The memory limit for the
Code Improvements
-
Edge Case Handling in
check-pr-content-policy.sh(Lines 83-88)- Issue: The script doesn't account for situations where no
BASE_SHAis provided but a diff is generated anyway (e.g., an empty repository state or invalid SHA). - File:
.ci/check-pr-content-policy.sh - Suggestion:
Add an explicit check for a validBASE_SHA:if [[ "$(git diff --name-only "$BASE_SHA" HEAD | wc -l)" -eq 0 ]]; then fatal "No changes detected between $BASE_SHA and HEAD. Ensure the base SHA is correct." fi
- Issue: The script doesn't account for situations where no
-
Regex Manager in Renovate Configuration
- Issue: The
managerFilePatternsproperty in.github/renovate.jsonis overly broad and may unintentionally attempt updates in unrelated YAML files. - File:
.github/renovate.json(Lines 7-17) - Suggestion: Narrow the pattern to exclude non-CI YAML files:
"managerFilePatterns": ["/^pipelines/.*\\.ya?ml$/", "/^prow-jobs/.*\\.ya?ml$/"]
- Issue: The
Best Practices
-
Testing Coverage for
check-pr-content-policy.sh- Issue: The script lacks unit tests for critical functions like
is_allowed_pingcap_hostandcollect_policy_reasons. - File:
.ci/check-pr-content-policy.sh - Suggestion: Add a suite of test cases using
batsor another Bash testing framework. Include tests for:- Valid and invalid hosts
- Content with forbidden substrings
- Multiple violations in a single line
- Issue: The script lacks unit tests for critical functions like
-
Documentation for New Presubmit Job
- Issue: No documentation exists for the new
pull-verify-pr-content-policyjob. - File:
prow-jobs/pingcap-qe/ci/presubmits.yaml - Suggestion:
Add a comment block at the top of the YAML file explaining the purpose of the job and how violations are reported. Example:# pull-verify-pr-content-policy: # Ensures PRs comply with repository content policy by checking added lines for forbidden substrings and hosts. # Violations are reported in the job logs.
- Issue: No documentation exists for the new
-
Consistency in Security Context Defaults
- Issue: Security context definitions for upgraded
flux-clicontainers are inconsistent across presubmit jobs. - Files:
prow-jobs/pingcap-qe/ci/presubmits.yaml(Line 131),prow-jobs/ti-community-infra/configs/presubmits.yaml(Lines 84, 112) - Suggestion:
Define shared security context properties globally or ensure consistency across jobs:securityContext: runAsUser: 0 allowPrivilegeEscalation: false
- Issue: Security context definitions for upgraded
Summary of Action Items
- Improve error handling and edge case validation in
check-pr-content-policy.sh. - Adjust memory limits for the new presubmit job.
- Narrow Renovate regex patterns to avoid unintended updates.
- Add unit tests for the new script.
- Document the new CI job and ensure consistent security configurations.
These changes will improve robustness, maintainability, and security of the CI pipeline and related scripts.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dillon-zheng The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the default Jenkins URL to prow.tidb.net/jenkins, introduces a script and Prow job to enforce PR content policies, and adds a Renovate custom manager for container image updates. It also bumps the flux-cli version in CI configurations. Review feedback suggests correcting a directory path in the Renovate config and improving the regex in the content policy script to handle word boundaries and the root domain correctly.
| "customType": "regex", | ||
| "datasourceTemplate": "docker", | ||
| "versioningTemplate": "docker", | ||
| "managerFilePatterns": ["/(^|/)(jobs/pipelines|prow-jobs)/.+\\.ya?ml$/"], |
There was a problem hiding this comment.
The managerFilePatterns for the custom Renovate manager seems to have a typo. It uses jobs/pipelines, but based on the repository structure and other CI configurations, the directory for pipeline Groovy and pod YAML files is simply pipelines/. This will cause Renovate to miss container image updates in those files.
| "managerFilePatterns": ["/(^|/)(jobs/pipelines|prow-jobs)/.+\\.ya?ml$/"], | |
| "managerFilePatterns": ["/(^|/)(pipelines|prow-jobs)/.+\\.ya?ml$/"], |
| continue | ||
| fi | ||
| append_unique_value "contains forbidden host ${lower_host}" "$reasons_name" | ||
| done < <(printf '%s\n' "$content" | grep -ioE "$PINGCAP_HOST_PATTERN" || true) |
There was a problem hiding this comment.
The current regex for matching pingcap.net hosts might lead to false positives if the pattern is part of a larger domain (e.g., foo.pingcap.net.example.com). Consider adding word boundaries to ensure it only matches full hostnames.
| done < <(printf '%s\n' "$content" | grep -ioE "$PINGCAP_HOST_PATTERN" || true) | |
| done < <(printf '%s\n' "$content" | grep -ioE "\\b$PINGCAP_HOST_PATTERN\\b" || true) |
|
|
||
| set -euo pipefail | ||
|
|
||
| readonly PINGCAP_HOST_PATTERN='([[:alnum:]-]+\.)+pingcap\.net' |
There was a problem hiding this comment.
The PINGCAP_HOST_PATTERN regex requires at least one subdomain (e.g., foo.pingcap.net). If the policy is intended to also restrict the root domain pingcap.net (unless explicitly allowed), the pattern should be adjusted to make the subdomain part optional.
| readonly PINGCAP_HOST_PATTERN='([[:alnum:]-]+\.)+pingcap\.net' | |
| readonly PINGCAP_HOST_PATTERN='([[:alnum:]-]+\\.)*pingcap\\.net' |
2 participants
This pull request introduces a new content policy check for pull requests, updates Jenkins URLs to a new domain, and improves container image management and security in CI pipelines. The major changes include adding a script and Prow job to enforce PR content rules, updating Jenkins references from the old to the new domain, enhancing Renovate configuration to update container images in YAML files, and upgrading the
flux-clicontainer image for better security and features.Pull Request Content Policy Enforcement:
.ci/check-pr-content-policy.sh, a script to check added lines in pull requests for forbidden substrings and unauthorizedpingcap.nethosts, with reporting and usage instructions.pull-verify-pr-content-policyinprow-jobs/pingcap-qe/ci/presubmits.yaml, ensuring all PRs tomainare validated.Jenkins URL Migration:
https://do.pingcap.net/jenkinstohttps://prow.tidb.net/jenkinsin scripts, documentation, and usage examples (.ci/replay-jenkins-build.sh,.agents/skills/test-jenkins-pipeline-changes-in-pr-by-replaying/SKILL.md). [1] [2] [3] [4] [5]Container Image Management Improvements:
.github/renovate.jsonwith a custom manager to automatically update container images referenced inpipelines/andprow-jobs/YAML files.CI Pipeline Security and Maintenance:
flux-clicontainer images in multiple Prow job configurations to versionv2.2.3for improved security and features (prow-jobs/pingcap-qe/ci/presubmits.yaml,prow-jobs/ti-community-infra/configs/presubmits.yaml). [1] [2] [3]The job is short-term for migration, it will be deprecated in future.