refactor(pingcap/tiflow,pingcap/tidb-tools): refactor the building of sync-diff-inspector tool #548

wuhuizuo · 2025-02-08T09:32:34Z

Changes

migrate building from pingcap/tidb-tools to pingcap/tiflow repo.
publish tiup package: sync-diff-inspector.
build and deliver image to docker.io/pingcap/sync-diff-inspector.
compose offline toolkit package with tiup package sync-diff-inspector rather than sync_diff_inspector raw binary file.

Close #546

… sync-diff-inspector tool - migrate building from `pingcap/tidb-tools` to `pingcap/tiflow` repo. - publish tiup package: `sync-diff-inspector`. - build and deliver image to `docker.io/pingcap/sync-diff-inspector`. - compose offline toolkit package with tiup package `sync-diff-inspector` rather than `sync_diff_inspector` raw binary file. Close #546 Signed-off-by: wuhuizuo <wuhuizuo@126.com>

ti-chi-bot · 2025-02-08T09:32:47Z

I have already done a preliminary review for you, and I hope to help you do a better job.

The pull request mainly focuses on refactoring the build process of the sync-diff-inspector tool, migrating it from pingcap/tidb-tools to the pingcap/tiflow repository. It also includes changes to publish the sync-diff-inspector package to the tiup package manager and deliver the image to docker.io/pingcap/sync-diff-inspector.

Key changes include:

A new Dockerfile for building sync-diff-inspector in the tiflow repository.
Modifications to packages/delivery.yaml to include the new image repository for sync-diff-inspector.
Changes in packages/offline-packages.yaml.tmpl and packages/packages.yaml.tmpl to include sync-diff-inspector in the package build process.
Replacing sync_diff_inspector raw binary file with sync-diff-inspector tiup package in the offline toolkit package.

Possible issues:

There are no visibility into the tests done after the refactoring. Ensure the refactoring doesn't break the existing function of the sync-diff-inspector tool.
The old sync_diff_inspector binary is removed. Ensure all references and dependencies on the old binary are updated to the new sync-diff-inspector tiup package.
When making such changes, it's necessary to ensure that all necessary documentation is updated to reflect the new build process and usage of the sync-diff-inspector tool.

Suggestions to fix:

Provide details on the testing done to ensure the refactored tool works as expected.
Double-check all references to the old sync_diff_inspector binary and update them to the new sync-diff-inspector tiup package.
Update all relevant documentation to reflect the new build process and usage instructions for the sync-diff-inspector tool.

wuhuizuo · 2025-02-08T09:33:35Z

dockerfiles/products/tiflow/sync-diff-inspector.Dockerfile

just copy from dockerfiles/products/tidb-tools/Dockerfile

wuhuizuo · 2025-02-08T09:35:24Z

packages/offline-packages.yaml.tmpl

+              - name: sync-diff-inspector # from raw binary to tiup pkg from v9.0.0
+                src: { type: tiup-clone, version: "{{ .Release.version }}" }


just this component is different with ">=8.4.0-0, < v9.0.0-0" route.

wuhuizuo · 2025-02-08T09:36:40Z

packages/offline-packages.yaml.tmpl

+                  # sync_diff_inspector stop builds after v8.5.1 on tidb-tools repo. So we use the latest v8.5.x version for patch version lower then v9.0.0.
+                  "{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"


for history patch version we pin the version as v8.5.1 that will be used to compose offline toolkit package.

packages/packages.yaml.tmpl

Signed-off-by: wuhuizuo <wuhuizuo@126.com>

ti-chi-bot · 2025-02-08T09:50:15Z

I have already done a preliminary review for you, and I hope to help you do a better job.

This pull request is to refactor the building of the sync-diff-inspector tool. The key changes are:

It migrates the building of the sync-diff-inspector tool from the pingcap/tidb-tools repository to the pingcap/tiflow repository.
The sync-diff-inspector tool is now published as a tiup package.
A new image is built and delivered to docker.io/pingcap/sync-diff-inspector.
The offline toolkit package now uses the sync-diff-inspector tiup package instead of the sync_diff_inspector raw binary file.

Potential problems:

The version v9.0.0 of tidb-tools is skipped in the scripts. If any components rely on this specific version, it could potentially cause a problem.
The new dockerfile for sync-diff-inspector is very barebones. It only copies the sync_diff_inspector binary into the image. If the binary requires any additional files or libraries, the image won't work as expected.

Fixing Suggestions:

Ensure all components that use tidb-tools are compatible with other versions or refactor them to not rely on the v9.0.0 version specifically.
Ensure that the sync-diff-inspector binary does not require any additional files or libraries to run. If it does, these should be included in the Dockerfile.

wuhuizuo · 2025-02-08T09:51:43Z

/hold

hold it for enough approvals.

packages/packages.yaml.tmpl

ti-chi-bot · 2025-02-08T09:53:36Z

I have already done a preliminary review for you, and I hope to help you do a better job.

The Pull Request (PR) is titled "refactor(pingcap/tiflow,pingcap/tidb-tools): refactor the building of sync-diff-inspector tool" and primarily deals with refactoring the build process of the sync-diff-inspector tool.

Key changes introduced in the PR are:

The building process of sync-diff-inspector has been migrated from pingcap/tidb-tools to pingcap/tiflow repo.
The sync-diff-inspector package has been published to tiup.
A new image has been built and delivered to docker.io/pingcap/sync-diff-inspector.
The offline toolkit package now composes with the tiup package sync-diff-inspector instead of the raw binary file sync_diff_inspector.
There are changes in the .github/scripts/ci.sh file to skip the tidb-tools for version v9.0.0.
A new Dockerfile for sync-diff-inspector has been added to the dockerfiles/products/tiflow directory.
Changes in packages/delivery.yaml for deprecated images and image copy rules.
The packages/offline-packages.yaml.tmpl and packages/packages.yaml.tmpl files have been updated to reflect the changes in the build process of sync-diff-inspector tool.

Potential Problems:

There might be potential issues with skipping tidb-tools for version v9.0.0. It's not clear from the PR why this is necessary and whether it could cause any side effects.
The image URL in packages/offline-packages.yaml.tmpl and packages/packages.yaml.tmpl has been hard coded to v8.5.1. This could cause problems if that specific version is not available or has been deprecated.

Fixing Suggestions:

Add comments in the code to clarify why tidb-tools is being skipped for version v9.0.0.
Instead of hard coding the version in the URL, consider using a variable to hold the version number. This would make it easier to update the version in the future.
It's not clear from the PR whether the new build process has been tested. It would be advisable to include test results or a description of the testing process in the PR description.
The PR modifies a significant number of files. It would be useful to provide more detailed explanations in the PR description of why each change is necessary.
Although the PR description states that it closes building sync_diff_inspector from tiflow repo #546, it would be helpful to provide a link to this issue for context and to make it easier for reviewers to understand the background of these changes.

wuhuizuo · 2025-02-08T09:53:44Z

packages/packages.yaml.tmpl

+                  path: bin/sync_diff_inspector
+            tiup:
+              description: >-
+                sync-diff-inspector is a tool used to verify the consistency across different MySQL-compatible data sources.


cc @joechenrh

wuhuizuo · 2025-02-08T13:43:25Z

packages/offline-packages.yaml.tmpl

-                  # version release on master branch.
-                  url: "{{ .Release.registry }}/pingcap/tidb-tools/package:master_{{ .Release.os }}_{{ .Release.arch }}"
+                  # sync_diff_inspector stop builds after v8.5.1 on tidb-tools repo. So we use the latest v8.5.x version for patch version lower then v9.0.0.
+                  "{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"


Suggested change

"{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"

url: "{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"

Signed-off-by: wuhuizuo <wuhuizuo@126.com>

ti-chi-bot · 2025-02-08T13:50:32Z

I have already done a preliminary review for you, and I hope to help you do a better job.

The pull request primarily revolves around the refactoring of the building process of the sync-diff-inspector tool. The key changes in the pull request are as follows:

The building process for the sync-diff-inspector tool has been migrated from pingcap/tidb-tools to pingcap/tiflow.
There's a new tiup package for sync-diff-inspector.
The pull request includes the building and delivering of an image to docker.io/pingcap/sync-diff-inspector.
The offline toolkit package is now composed with the tiup package sync-diff-inspector rather than the raw binary file sync_diff_inspector.
There is a new Dockerfile for the sync-diff-inspector tool, that copies the sync_diff_inspector binary into the Docker image.
The tidb-tools component is skipped for version v9.0.0 in the GitHub CI script.
The pull request includes updates to the delivery rules and offline package templates to reflect the aforementioned changes.

Potential Problems:

The sync-diff-inspector binary is being copied directly into the Docker image. If the binary has any dependencies that are not present in the Docker image, this could lead to runtime errors.
The tidb-tools component is skipped for version v9.0.0 in the GitHub CI script. If there are any dependencies on this component for this version, this could potentially cause issues.
There are several hard-coded version numbers in the changes, this could cause problems if these versions become outdated or need to be updated frequently.

Suggestions to fix:

If the sync-diff-inspector binary has any dependencies, make sure they are included in the Docker image or handled appropriately.
If there are any dependencies on the tidb-tools component for version v9.0.0, consider addressing them to avoid potential issues.
Consider using variables for version numbers or any other values that may change frequently, to avoid having to manually update them each time.

joechenrh · 2025-02-12T02:08:24Z

.github/scripts/ci.sh

    for cm in $components; do
        for version in $versions; do
+            # Skip tidb-tools for version v9.0.0
+            if [[ $cm == "tidb-tools" && $version == "v9.0.0" ]]; then


We only skip for version 9.0.0?

No, It's just a CI test script, Now the biggest version in test matrix is v9.0.0.

Thanks for you question.

ti-chi-bot · 2025-02-17T03:06:02Z

@joechenrh: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Benjamin2037

LGTM

ti-chi-bot · 2025-02-17T03:15:08Z

@Benjamin2037: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

wuhuizuo · 2025-02-17T03:24:34Z

/unhold

purelind

/lgtm

ti-chi-bot · 2025-02-17T03:38:25Z

[LGTM Timeline notifier]

Timeline:

2025-02-17 03:38:25.073372473 +0000 UTC m=+846147.469594535: ☑️ agreed by purelind.

wuhuizuo · 2025-02-17T04:05:48Z

/approve

ti-chi-bot · 2025-02-17T04:05:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, joechenrh, purelind, wuhuizuo

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:

~~OWNERS~~ [wuhuizuo]

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 bot requested a review from purelind February 8, 2025 09:32

ti-chi-bot bot added the size/L label Feb 8, 2025

wuhuizuo commented Feb 8, 2025

View reviewed changes

dockerfiles/products/tiflow/sync-diff-inspector.Dockerfile

Copy link

Contributor Author

wuhuizuo Feb 8, 2025

Choose a reason for hiding this comment

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

just copy from dockerfiles/products/tidb-tools/Dockerfile

wuhuizuo commented Feb 8, 2025

View reviewed changes

packages/packages.yaml.tmpl Outdated Show resolved Hide resolved

wuhuizuo mentioned this pull request Feb 8, 2025

sync-diff-inspector: Update README pingcap/tidb-tools#848

Open

ci: fix ci script to solve the tidb-tools building change

af6b36d

Signed-off-by: wuhuizuo <wuhuizuo@126.com>

ti-chi-bot bot added size/XL and removed size/L labels Feb 8, 2025

ti-chi-bot bot added the do-not-merge/hold label Feb 8, 2025

wuhuizuo commented Feb 8, 2025

View reviewed changes

packages/packages.yaml.tmpl Outdated Show resolved Hide resolved

Update packages/packages.yaml.tmpl

a228056

wuhuizuo commented Feb 8, 2025

View reviewed changes

fix(packages): fix offline packages config

d2abb8a

Signed-off-by: wuhuizuo <wuhuizuo@126.com>

joechenrh reviewed Feb 12, 2025

View reviewed changes

joechenrh approved these changes Feb 17, 2025

View reviewed changes

Benjamin2037 approved these changes Feb 17, 2025

View reviewed changes

ti-chi-bot bot removed the do-not-merge/hold label Feb 17, 2025

purelind approved these changes Feb 17, 2025

View reviewed changes

ti-chi-bot bot added the lgtm label Feb 17, 2025

ti-chi-bot bot added the approved label Feb 17, 2025

ti-chi-bot bot merged commit 8f211b5 into main Feb 17, 2025
3 checks passed

ti-chi-bot bot deleted the wuhuizuo/issue546 branch February 17, 2025 04:08

		- name: sync-diff-inspector # from raw binary to tiup pkg from v9.0.0
		src: { type: tiup-clone, version: "{{ .Release.version }}" }

		# sync_diff_inspector stop builds after v8.5.1 on tidb-tools repo. So we use the latest v8.5.x version for patch version lower then v9.0.0.
		"{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"

	"{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"
	url: "{{ .Release.registry }}/pingcap/tidb-tools/package:v8.5.1_{{.Release.os }}_{{ .Release.arch }}"

refactor(pingcap/tiflow,pingcap/tidb-tools): refactor the building of sync-diff-inspector tool #548

refactor(pingcap/tiflow,pingcap/tidb-tools): refactor the building of sync-diff-inspector tool #548

Uh oh!

Conversation

wuhuizuo commented Feb 8, 2025

Changes

Uh oh!

ti-chi-bot bot commented Feb 8, 2025

Uh oh!

wuhuizuo Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

wuhuizuo Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

wuhuizuo Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-chi-bot bot commented Feb 8, 2025

Uh oh!

wuhuizuo commented Feb 8, 2025

Uh oh!

Uh oh!

ti-chi-bot bot commented Feb 8, 2025

Uh oh!

wuhuizuo Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

wuhuizuo Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 8, 2025

Uh oh!

joechenrh Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

wuhuizuo Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 17, 2025

Uh oh!

Benjamin2037 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 17, 2025

Uh oh!

wuhuizuo commented Feb 17, 2025

Uh oh!

purelind left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 17, 2025

[LGTM Timeline notifier]

Uh oh!

wuhuizuo commented Feb 17, 2025

Uh oh!

ti-chi-bot bot commented Feb 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants