feat(packages): add nextgen tiflow route and delivery#957
feat(packages): add nextgen tiflow route and delivery#957ti-chi-bot[bot] merged 2 commits intomainfrom
Conversation
![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 nextgen support for the tiflow package route and delivery, including related images dm, tiflow, and sync-diff-inspector under the tidbx registry path. It updates package build profiles, delivery rules, and local validation scripts to reflect these additions. The changes are well-scoped, address the stated issue (#956), and verification steps indicate correctness. The overall structure is clear, but some areas could improve on maintainability and robustness.
Critical Issues
- None found. The code changes appear functionally correct based on the description and diff.
Code Improvements
-
packages.yaml.tmpl(lines ~2244-2295):
The shell script in thenextgenprofile step installs Node.js and yarn manually. This approach may fail silently or be slow on repeated runs, and it lacks error handling.Suggestion:
- Add error checking after critical commands (
curl,nvm install,nvm use,npm install). - Consider caching or detecting if the right Node.js version is already installed to avoid redundant installs.
- Alternatively, document that this step assumes a clean environment.
Example improvement snippet:
set -e if ! command -v node >/dev/null || [ "$(node -v)" != "$NODE_VERSION" ]; then # install Node/NVM # ... fi
- Add error checking after critical commands (
-
get-delivery-target-images.ts(line ~38):
The addition of"pingcap/tiflow/images/sync-diff-inspector"is good, but consider making this list data-driven or configurable to reduce manual enumeration and risk of future drift.For example, define an array or config object at the top and reference it here and elsewhere.
-
delivery.yaml(lines ~487-500):
The new delivery rules duplicate the pattern in other rules. If morenextgenimages are added in the future, this could be error-prone.Consider templating or using YAML anchors more extensively to DRY up repeated structures.
Best Practices
-
Documentation
- Add a brief comment explaining the purpose of the new
nextgenprofile and delivery rules inpackages.yaml.tmplanddelivery.yamlfor future maintainers. - Document the environment expectations for the node bootstrap step (e.g., network access for
curl).
- Add a brief comment explaining the purpose of the new
-
Testing Coverage
- The PR description lists manual verification steps but does not mention automated tests.
- Add or update automated tests (unit or integration) to cover:
- The new
nextgenprofile inpackages.yaml.tmpl. - The updated image enumeration in
get-delivery-target-images.ts. - Delivery rules validation if possible.
- The new
-
Style / Naming
- The naming is consistent and clear.
- In the shell script, consider quoting variables for robustness:
and
mkdir -p "$NVM_DIR"if [ ! -s "$NVM_DIR/nvm.sh" ]; then
Summary of Suggested Fixes
# packages/packages.yaml.tmpl - nextgen node bootstrap with error handling and quoting
+ - script: |
+ set -e
+ NODE_VERSION="v16.20.2"
+ NVM_VERSION="v0.39.5"
+ NVM_DIR="$HOME/.nvm"
+ mkdir -p "$NVM_DIR"
+
+ if [ ! -s "$NVM_DIR/nvm.sh" ]; then
+ curl -fsSL "https://raw.githubusercontent.com/nvm-sh/nvm/${NVM_VERSION}/install.sh" | bash
+ fi
+ . "$NVM_DIR/nvm.sh"
+
+ # Install node if not already installed or wrong version
+ if ! command -v node >/dev/null || [ "$(node -v)" != "$NODE_VERSION" ]; then
+ nvm install "${NODE_VERSION}"
+ fi
+ nvm use "${NODE_VERSION}"
+ nvm alias default "${NODE_VERSION}"
+
+ node --version && npm --version
+ npm install -g yarn- Add comments in
delivery.yamlandpackages.yaml.tmplexplaining new nextgen additions. - Refactor
get-delivery-target-images.tsto use a central config for image paths to avoid manual sync issues. - Add automated tests for the new functionality.
Overall, the PR is a solid incremental feature addition. Addressing the above points will improve robustness, maintainability, and clarity for future contributors.
![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 updates the delivery and packaging configuration for tiflow components by adding image copy rules for dm, tiflow, and sync-diff-inspector. It also introduces a nextgen profile in the package template, which includes a Node.js toolchain setup for Darwin environments. A review comment suggests reducing code duplication in the package template by using YAML anchors or Go template blocks for the repeated Node.js installation steps.
packages/packages.yaml.tmpl
Outdated
| nextgen: | ||
| - os: darwin | ||
| description: install nodejs toolchain. | ||
| script: | | ||
| NODE_VERSION="v16.20.2" | ||
| NVM_VERSION="v0.39.5" | ||
| NVM_DIR="$HOME/.nvm" | ||
| mkdir -p $NVM_DIR | ||
|
|
||
| if [ ! -s "$NVM_DIR/nvm.sh" ]; then | ||
| curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/${NVM_VERSION}/install.sh | bash | ||
| fi | ||
| . $NVM_DIR/nvm.sh | ||
| nvm install ${NODE_VERSION} | ||
| nvm use ${NODE_VERSION} | ||
| nvm alias default ${NODE_VERSION} | ||
|
|
||
| node --version && npm --version | ||
| npm install -g yarn | ||
| - script: | | ||
| NEXT_GEN=1 make dm-master-with-webui dm-worker dmctl dm-syncer sync-diff-inspector |
There was a problem hiding this comment.
The nextgen profile steps for darwin are identical to the release profile steps. While this follows the existing pattern in this file, it introduces significant code duplication. Consider defining a reusable YAML anchor or a Go template block for the Node.js toolchain installation to improve maintainability.
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 support for a new nextgen profile in the tiflow package build and delivery system, integrating it alongside the existing release profile. It updates delivery rules to include nextgen variants for tiflow, dm, and sync-diff-inspector images targeting the tidbx registry, and modifies the packaging templates to generate appropriate artifacts and image tags. The approach is consistent and incremental, maintaining backward compatibility with the release profile. The code changes are generally clear and well-structured, but some improvements and clarifications would enhance maintainability and robustness.
Critical Issues
- Potential Missing Delivery Rule for
sync-diff-inspectorindelivery.yaml
Inpackages/delivery.yaml, the PR adds a delivery rule fordmunder thetidbxregistry path but does not explicitly add one forsync-diff-inspectorortiflowunderus-docker.pkg.dev/pingcap-testing-account/tidbx. The description mentions adding nextgen delivery rules for all three images, but the diff only showsdm.- File:
packages/delivery.yamllines ~487-491 - Issue: This could lead to missing delivery targets for
sync-diff-inspectorandtiflowimages in thetidbxpath, causing inconsistencies or failed deployments. - Suggestion: Add similar delivery rules for
sync-diff-inspectorandtiflowunder the appropriate source repository keys, e.g.:us-docker.pkg.dev/pingcap-testing-account/tidbx/pingcap/tiflow/images/sync-diff-inspector: - <<: *sync_nextgen dest_repositories: - us.gcr.io/pingcap-public/tidbx/sync-diff-inspector us-docker.pkg.dev/pingcap-testing-account/tidbx/pingcap/tiflow/images/tiflow: - <<: *sync_nextgen dest_repositories: - us.gcr.io/pingcap-public/tidbx/tiflow
- File:
Code Improvements
-
DRY and Consistency in
packages.yaml.tmplProfile Conditions
The current changes addnextgento the profile list at line 2246 and separately excludenextgenartifacts viaif: {{ ne "nextgen" .Release.profile }}conditions at lines 2327, 2339, and 2362. This pattern risks duplication and errors if more artifact sections are added later.- File:
packages/packages.yaml.tmpllines 2244-2370 - Issue: The exclusion logic is repeated for multiple artifacts and can be error-prone if the
nextgenprofile changes. - Suggestion: Consider grouping
ifconditions or using a single variable for profile-based artifact filtering. For example, define a template variable likeisNextGenonce and reuse it. This improves readability and reduces duplication.
- File:
-
Hardcoded Node.js and NVM Versions in Nextgen Profile Setup
Thenextgenprofile bootstrap script installs specific versions of Node.js (v16.20.2) and NVM (v0.39.5) directly.- File:
packages/packages.yaml.tmpllines ~2250-2268 - Issue: Hardcoding versions makes upgrades and maintenance more cumbersome. These versions may become outdated and cause environment drift.
- Suggestion: Extract these versions into variables at the top of the template or external config, or parameterize them with environment variables. This makes upgrades easier and avoids potential security issues from old Node.js versions.
- File:
-
Missing
sync-diff-inspectorin Nextgen Make Targets
In thenextgenprofile build step, the make targets are:NEXT_GEN=1 make dm-master-with-webui dm-worker dmctl dm-syncer
However,
sync-diff-inspectoris present in thereleaseprofile but absent here.- File:
packages/packages.yaml.tmpllines ~2269-2274 - Issue: Omitting
sync-diff-inspectorfrom thenextgenmake targets may cause it not to be built or delivered under nextgen, contradicting the PR description. - Suggestion: Add
sync-diff-inspectorto the make command in thenextgenprofile:NEXT_GEN=1 make dm-master-with-webui dm-worker dmctl dm-syncer sync-diff-inspector
- File:
Best Practices
-
Add Comments Explaining
nextgenProfile Purpose and Differences
The template additions introduce a newnextgenprofile with some nodejs bootstrap and altered build targets, but there is no inline documentation explaining whatnextgenrepresents or how it differs fromrelease.- File:
packages/packages.yaml.tmplstarting line 2240 and 2250 - Issue: Lack of comments makes it harder for future maintainers to understand design intent.
- Suggestion: Add comments briefly describing the
nextgenprofile, its intended usage, and why Node.js bootstrap is necessary.
- File:
-
Consistent Use of Profile Filtering in Artifacts
The artifact definitions forsync-diff-inspectorandtiflow-engineimages explicitly excludenextgenprofile artifacts. This is good but should be consistently applied across all relevant artifact blocks.- File:
packages/packages.yaml.tmpllines 2326-2365 - Suggestion: Review all artifact blocks to ensure correct filtering and consider centralizing profile-based filtering logic for maintainability.
- File:
-
Testing Coverage
The PR description shows manual verification commands but does not mention automated tests for the newnextgenprofile or delivery rules.- Suggestion: Add or update automated tests (unit, integration, or end-to-end) that verify the
nextgenbuild and delivery paths, including image enumeration and delivery rule application. This reduces regression risk.
- Suggestion: Add or update automated tests (unit, integration, or end-to-end) that verify the
Summary of Recommended Changes:
- Add missing delivery rules for
sync-diff-inspectorandtiflowinpackages/delivery.yamlfornextgen/tidbxregistry paths. - Include
sync-diff-inspectorin thenextgenmake targets inpackages.yaml.tmpl. - Refactor artifact filtering logic by profile to reduce duplication and improve clarity.
- Parameterize Node.js and NVM version variables in the bootstrap script.
- Add comments explaining the purpose and setup steps of the
nextgenprofile. - Add or update automated tests covering the new
nextgenprofile and delivery rules.
Addressing these points will improve correctness, maintainability, and reduce potential future issues.
|
[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:
|
2 participants
Summary
nextgensupport to the latesttiflowpackage route and keep the darwin nodejs bootstrap in the new profiletiflow,dm, andsync-diff-inspectorimages in thetidbxregistry pathsync-diff-inspectorin the delivery target image enumeration helper so local delivery validation matches the configured rulesCloses #956
Verification
gen-package-images-with-config.sh tiflow linux amd64 v9.0.0 nextgen ...failed withNo package routes matchedget-delivery-target-images.ts --version=v26.3.1-nextgen --registry=us-docker.pkg.dev/pingcap-testing-account/tidbxdid not includedm/tiflow/sync-diff-inspectortargetsgen-package-images-with-config.shcommand generates a script withNEXT_GEN=1 make dm-master-with-webui dm-worker dmctl dm-syncer sync-diff-inspectorandtidbximage destinations for all three tiflow-family imagesus.gcr.io/pingcap-public/tidbx/dm:v26.3.1-nextgen,.../tiflow:v26.3.1-nextgen, and.../sync-diff-inspector:v26.3.1-nextgenreleasegeneration still emits the classichub/pingcap/tiflow/images/{sync-diff-inspector,dm,tiflow}destinations