feat: add new component in packages.yaml.tmpl for building tikv/migration image#907
Conversation
image Signed-off-by: lyb <yebin.li@pingcap.com>
Signed-off-by: lyb <yebin.li@pingcap.com>
![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 PR adds a new migration component in the packages.yaml.tmpl file, enabling building and releasing Docker images for tikv-br, tikv-cdc, and kafka-consumer. It also updates the CI shell script to include the migration component in relevant build and test flows. The approach is straightforward, extending existing patterns to a new component, and the changes appear coherent and consistent with current code style.
Critical Issues
-
Potential CI script version/compatibility issues
- File:
.github/scripts/ci.shlines ~23, ~294 - Issue: The
migrationcomponent is added to the components list, but there is no conditional skip logic like fortidb-toolsat versionv9.0.0. If any special handling is needed formigrationin CI (e.g., version exclusions or compatibility checks), it is not present. - Suggestion: Confirm if
migrationrequires any special conditional handling similar totidb-tools. If so, add appropriate skips or version guards.
- File:
-
Hardcoded builder image version
- File:
packages/packages.yaml.tmpllines ~3269-3274 - Issue: The
migrationbuilder image uses a fixed tagv2026.2.1-3-gb0438c3. This may cause issues with reproducibility or updates if not maintained appropriately. - Suggestion: Consider templating this tag or documenting update requirements. For example:
builders: - image: ghcr.io/pingcap-qe/cd/utils/release:{{ .Builders.migration_tag | default "v2026.2.1-3-gb0438c3" }}
- File:
Code Improvements
-
Improve consistency with existing components
- File:
packages/packages.yaml.tmpllines ~3263-3299 - Issue: The
migrationcomponent description and formatting slightly differ from other components (e.g., no indentation onmigration:key, and thedescfield uses a lowercase "tikv/migration" while others begin with uppercase). - Suggestion: Follow existing styling conventions, capitalize description first letter, and ensure consistent indentation:
migration: desc: Tikv/Migration - tikv-br, tikv-cdc, Kafka-consumer git: # ...
- File:
-
Dockerfile paths clarity
- File:
packages/packages.yaml.tmpllines ~3288-3299 - Issue: The
kafka-consumerimage Dockerfile path iscdc/deployments/tikv-cdc/docker/kafka-consumer.Dockerfilewhich differs in filename style (capitalized.Dockerfile). Ensure this naming is correct and consistent with project conventions to avoid confusion or build errors. - Suggestion: Verify dockerfile naming conventions and align with existing ones like
Dockerfileordockerfile.
- File:
Best Practices
-
Add comments to CI script updates
- File:
.github/scripts/ci.shlines ~23 and ~294 - Issue: Components list is updated in two places. There is no comment explaining why
migrationis added or its scope in the build/test matrix. - Suggestion: Add brief comments to clarify the addition, e.g.:
# Added migration component for tikv migration image builds local components="tidb tiflow tiflash tikv pd ctl monitoring ng-monitoring tidb-tools migration"
- File:
-
Testing coverage
- The PR description references closing an issue but does not mention any added or updated tests for the new migration component.
- Suggestion: Ensure there are corresponding tests or CI jobs validating the new migration component images are built correctly. If not present, add integration tests or smoke tests to verify image build success.
Overall, the PR is a well-structured addition of a new component with minor areas for improvement in documentation, consistency, and CI robustness. Addressing these points will improve maintainability and reliability.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds a new migration component to the build configuration. The changes include updating a test script to include this new component and defining the component's build properties in packages.yaml.tmpl. My review focuses on the new component definition. I've identified an issue with image tagging which could lead to builds overwriting each other and have suggested a fix to align with existing conventions and ensure build uniqueness.
Signed-off-by: lyb <yebin.li@pingcap.com>
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 introduces a new component migration in packages.yaml.tmpl for building TiKV/migration related images (tikv-br, tikv-cdc, kafka-consumer) and adds corresponding CI test invocations in .github/scripts/ci.sh. The approach extends existing patterns for adding new components with OS/architecture/version loops and validation checks. Overall, the additions are straightforward and consistent with existing code style, but some improvements in test robustness and template clarity could be made.
Critical Issues
-
CI Script Testing Invocation May Be Incomplete or Incorrect
In.github/scripts/ci.shat lines ~399-410, the script runs the migration build with fixed dummy branch and sha parameters:$script "$cm" linux "$ac" "$version" "$profile" branch-xxx 123456789abcdef shellcheck -S error packages/scripts/build-package-images.sh
- The use of fixed
"branch-xxx"and"123456789abcdef"without context may cause tests to not reflect real scenarios or mask errors. - Running
shellcheckon the build script here seems unrelated to the migration build test and might be misplaced.
Suggestion:
- Use realistic or parameterized git refs and shas matching the new component context or derive them dynamically.
- Move the
shellcheckinvocation out of the loop or into a dedicated linting step. - Add error checking after
$scriptinvocation to catch failures explicitly.
- The use of fixed
Code Improvements
-
Add Variable for Supported Architectures in
packages.yaml.tmpl
The migration component supportsamd64andarm64, but these architectures are hardcoded in the routers section. It would be better to define supported architectures as a variable or top-level parameter in the template to avoid duplication and ease future maintenance. -
Clarify Dockerfile Context and Paths in Migration Component
The template defines three artifacts with different contexts and dockerfiles:context: br dockerfile: deployments/docker/Dockerfile ... context: cdc dockerfile: deployments/tikv-cdc/docker/Dockerfile ... dockerfile: deployments/tikv-cdc/docker/kafka-consumer.Dockerfile
- It's not clear if these relative paths are correct or if the contexts align with dockerfile locations.
- Consider adding comments or validating paths to avoid build failures due to incorrect contexts.
-
Improve Shell Script Logging
Theecho -enlines in.github/scripts/ci.shcould be improved by adding timestamps or more descriptive logs to facilitate debugging. -
Error Handling for Image Existence Check
check_image_existed $imgis called but no failure handling is shown. Consider adding explicit exit or retry logic if image check fails.
Best Practices
-
Lack of Documentation in Template
The new migration component inpackages.yaml.tmpllacks comments explaining what each field means, especially theroutersandbuilderssections. Adding brief comments would aid maintainability. -
Test Coverage for Migration Component
The CI script adds basic invocation tests but does not show any integration or smoke tests specific to migration images. Consider adding targeted tests in the CI pipeline to verify functionality of built migration images. -
Naming Consistency
In the shell script, variablecmis used for the component name. This is consistent with other components but consider renaming it tocomponentfor clarity. -
Remove Unnecessary
shellcheckCall Inside Loop
Runningshellcheckinside the for loop for every architecture is redundant and slows down CI. Move it outside the loop or to a separate lint stage.
Summary of Actionable Suggestions
# In .github/scripts/ci.sh around line 400
for ac in $architectures; do
echo -en "[📃💿] $cm $os $ac $version $profile:\t"
img=$($script "$cm" linux "$ac" "$version" "$profile" "$GIT_REF" "$GIT_SHA")
if ! check_image_existed $img; then
echo "Image check failed for $img"
exit 1
fi
done
# Move shellcheck out of the loop
shellcheck -S error packages/scripts/build-package-images.sh# In packages/packages.yaml.tmpl
# Add comment about migration component
# migration: builds tikv-br, tikv-cdc and kafka-consumer images for TiKV migration tools.
migration:
desc: tikv/migration - tikv-br, tikv-cdc, kafka-consumer
git:
url: '{{ .Git.url | default "https://github.com/tikv/migration.git" }}'
ref: {{ .Git.ref | default "main" }}
sha: {{ .Git.sha | default "" }}
version: {{ .Release.version }}
artifactory:
tags:
{{- if .Git.sha }}
- {{ strings.ReplaceAll "/" "-" .Git.ref | strings.ToLower }}-{{ strings.Trunc 7 .Git.sha }}
{{- end }}
- {{ strings.ReplaceAll "/" "-" .Git.ref | strings.ToLower }}
- {{ .Release.version }}
- latest
builders:
- image: ghcr.io/pingcap-qe/cd/utils/release:v2026.2.1-3-gb0438c3
routers:
- description: main branch docker images
os: [linux]
arch: [amd64, arm64] # Consider defining this list as a variable
profile: [release]
artifacts:
# Add comment about each artifact's context/dockerfile paths
- name: tikv-br image
type: image
artifactory:
repo: "{{ .Release.registry }}/tikv/migration/images/tikv-br"
context: br
dockerfile: deployments/docker/Dockerfile
- name: tikv-cdc image
type: image
artifactory:
repo: "{{ .Release.registry }}/tikv/migration/images/tikv-cdc"
context: cdc
dockerfile: deployments/tikv-cdc/docker/Dockerfile
- name: kafka-consumer image
type: image
artifactory:
repo: "{{ .Release.registry }}/tikv/migration/images/kafka-consumer"
context: cdc
dockerfile: deployments/tikv-cdc/docker/kafka-consumer.DockerfileAddressing these points will improve test reliability, maintainability of the new component, and clarity for future contributors.
[LGTM Timeline notifier]Timeline:
|
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo 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 |
…gration-docker-release (#4287) Depends on: PingCAP-QE/artifacts#907 Close: #3347 --------- Signed-off-by: lyb <yebin.li@pingcap.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2 participants
Close: PingCAP-QE/ci#3347