Repository navigation
chore(cd): rebuild all CentOS 7 Go builder images - #1115
ti-chi-bot[bot] merged 1 commit into
Conversation
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 PR aims to trigger a full rebuild of all seven CentOS 7 Go builder images by inserting a single blank line in each skaffold.centos7.yaml file for those components. This is a minimal, whitespace-only change designed to force CI and release workflows to pick up and publish all builders, not just the previously fixed TiDBx one. The approach is sound and low-risk since it does not alter any actual build configuration or logic. The PR is well-documented and validated for YAML parsing and workflow selection. Overall, this is a clean and effective way to restart the full build matrix.
Code Improvements
-
Unnecessary use of whitespace change to trigger rebuilds
- Files: All changed
skaffold.centos7.yamlfiles (e.g.,dockerfiles/cd/builders/ng-monitoring/skaffold.centos7.yamland others) - Why: While the whitespace-only blank line insertions trigger rebuilds, this approach can be confusing to reviewers and future maintainers since it does not explicitly document the intent in the YAML or workflow files. It relies on side-effect rather than explicit configuration.
- Suggestion: Consider adding a dedicated flag or comment in the YAML or the CI workflow to indicate a forced rebuild rather than whitespace hacks. For example:
Or update workflow logic to support a manual "rebuild all" trigger event or input parameter, improving clarity and maintainability.
# Force rebuild trigger: true
- Files: All changed
-
No validation that CI actually triggers rebuilds of all images
- Why: The PR description states that image builds are performed by PR CI and release workflow, but no evidence (logs, artifacts links) is provided that all 21 component/profile jobs were successfully triggered and ran.
- Suggestion: Include links or outputs from the triggered workflow run showing all jobs started and completed, or add a smoke test step to validate all images are rebuilt and pushed as expected.
Best Practices
-
Documentation could be clearer in YAML files
- Files: All changed
skaffold.centos7.yamlfiles - Why: The blank lines added are not self-explanatory. New maintainers might not understand why these whitespace changes exist or what triggers the rebuilds.
- Suggestion: Add an inline comment above or below the blank line, e.g.:
This improves transparency and future troubleshooting.
tryImportMissing: true # Blank line added to force CI rebuild of this component
- Files: All changed
-
No tests or CI validation for rebuild trigger logic
- Why: Since the PR is whitespace-only, there is no automated test coverage or validation that the forced rebuild actually happens.
- Suggestion: Implement a CI job or workflow step that explicitly checks which builders are selected and built, comparing against the expected list. This would help catch regressions in rebuild triggering logic.
Summary of Actionable Feedback
- Avoid relying on whitespace-only changes to trigger rebuilds; instead, introduce explicit flags or parameters in YAML or workflows to control rebuilds.
- Add inline comments in each YAML file where blank lines are inserted to document the purpose of the change.
- Consider adding or linking to CI outputs demonstrating that all expected build jobs are triggered and completed.
- Implement CI validation steps to verify rebuild triggers and selection logic, preventing silent failures or missed rebuilds in the future.
This PR is low-risk and well-documented but would benefit from explicitness and improved observability in the rebuild triggering mechanism.
|
/approve |
|
[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 |
Purpose
Rebuild and publish the full seven-component CentOS 7 Go-builder matrix using the repaired code, rather than stopping after TiDBx alone was recovered.
e392e375and successfully published only TiDBx: https://github.com/PingCAP-QE/artifacts/actions/runs/38872606668 . It did not recover the original action's other cancelled components.29710e1SHA, without the fix. The existingworkflow_dispatchparser also does not support that event, so a manual dispatch is not a working full-build recovery path without broader workflow changes.Change
Seven whitespace-only insertions, one blank line per existing
skaffold.centos7.yaml:The parsed configurations are unchanged. The merge push matches the existing release paths and changed-builder parser, selecting all seven builders for Go 1.27 / 1.26 / 1.25, including root + non-root images and linux/amd64 + linux/arm64.
This produces the existing 21 component/profile publish jobs, each building both image variants for both architectures. The branch starts from
upstream/mainafter the TiDBx repository repair.No Dockerfile, version, profile, workflow logic, Rust/C++ builder, routing, or generated release-file change is included.
Validation
e392e375; fetched/rebased against upstream before submission.git diff --check.Runtime line coverage: not measured (whitespace-only trigger). Full image builds are performed by PR CI and, after merge, the release workflow; no publication success or future tag is claimed yet.
Completion boundary
PR checks use
--push=false; merging this trigger PR is what starts the full publication run. Record exact tags from that successful publish run, verify final root/non-root manifest lists contain both architectures and the expected per-architecture Go labels, and only then consider the builder publication complete. No downstream routing changes are made here.Leave unmerged for user review; auto-merge is not enabled.