-
Notifications
You must be signed in to change notification settings - Fork 110
fix(scripts): update ticdc publishing threshold to v8.5.4 #3920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Bump previous/current release to v8.5.3 and v8.5.4 Make collect-info-for-release-issue.sh executable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the publishing threshold for the ticdc component from version v9.0.0 to v8.5.4 across multiple scripts related to release automation and image/tag management. It also updates the previous/current release versions to v8.5.3 and v8.5.4 in the collect-info-for-release-issue.sh script and makes that script executable. The changes are straightforward version bumps and permission fix with consistent updates across relevant files. Overall, the PR appears focused and well-aligned with the intended release version update.
Critical Issues
- None identified. The version comparisons and changes appear logically consistent and correct.
Code Improvements
-
Version Comparison Consistency:
Several scripts use string comparisons likeversion <= "v8.5.4"or version sorting viasort -V. While this works for the current version format, consider centralizing version comparison logic into a helper function or utility script to avoid subtle bugs with string comparisons if version formats evolve (e.g.,v8.10.0might be miscompared). This is not critical here but could improve maintainability. -
Shell Script Safety:
Intag-rc2ga-on-repos.sh, version checks use:if [[ "$(printf '%s\n' "v8.5.4" "$ga_ver" | sort -V | head -n1)" == "v8.5.4" ]]; then
This is a good approach, but consider adding comments to explain the logic for future maintainers or wrapping it in a function with a descriptive name (e.g.,
version_gte "v8.5.4" "$ga_ver").
Best Practices
-
Documentation:
- In
scripts/flow/rc/check-images-internal.tsandcheck-tiup.ts, fix the comment typo:Should be// ticdc is initilized since v8.5.4initialized.
- In
-
Executable Flag:
- The PR makes
collect-info-for-release-issue.shexecutable, which is good. Ensure any other scripts modified or added in the future also have correct executable permissions.
- The PR makes
-
Comments Update:
- In
collect-info-for-release-issue.sh, update or clarify the comment:This comment could be confusing; consider explaining the exact condition or removing it if no longer relevant.previous_release="v8.5.3" # comment it when current release is not a patch release.
- In
-
Testing Coverage:
- No test files were changed or added. It would be good to ensure these version changes are covered by integration or unit tests that verify the correct behavior for different versions, especially for scripts involved in release processes.
Summary of actionable suggestions:
- In `scripts/flow/rc/check-images-internal.ts` line 155 and `check-tiup.ts` line 220, fix typo:
```ts
// ticdc is initialized since v8.5.4-
Consider wrapping version comparison logic in shell scripts (
tag-rc2ga-on-repos.sh) into a helper function, e.g.:version_gte() { # returns 0 if $2 >= $1 [[ "$(printf '%s\n' "$1" "$2" | sort -V | head -n1)" == "$1" ]] } # usage: if version_gte "v8.5.4" "$ga_ver"; then ... fi
-
Clarify or update the comment regarding
previous_releaseincollect-info-for-release-issue.shfor better clarity. -
Verify or add automated tests if possible to cover these version threshold changes, ensuring no regressions in release scripting workflows.
No critical issues found; mostly minor polish and maintainability improvements suggested.
Summary of ChangesHello @wuhuizuo, 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 synchronizes the project's release management scripts with the actual publishing schedule for TiCDC artifacts and images. It updates the version thresholds in several key scripts, ensuring that TiCDC components are correctly recognized and processed as part of the release pipeline starting from version Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the TiCDC publishing threshold version to v8.5.4 across several scripts. The changes are mostly correct, but I've identified a few areas for improvement:
- Bash scripts: The version comparison logic in
tag-rc2ga-on-repos.shis a bit confusing and inconsistent. I've suggested a refactoring to make it more readable and maintainable. - TypeScript scripts: The version comparisons in
check-images-internal.tsandcheck-tiup.tsuse string comparison, which is unreliable for semantic versions and can lead to bugs. Incheck-tiup.ts, there's also a logical error in a boundary condition that would cause it to skip the exact version it should start processing. I've provided comments and a suggestion to fix these high-severity issues.
Overall, the changes are in the right direction, but addressing the feedback will improve the correctness and robustness of these scripts.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the version gating logic for the pingcap/ticdc repository in multiple release and artifact management scripts, lowering the threshold from v9.0.0 to v8.5.4. The change is consistently applied across Bash and TypeScript scripts handling image tagging, release info collection, and initialization checks. Overall, the changes are straightforward, targeted, and mostly consistent. The version comparisons and comments are updated appropriately, improving accuracy for handling ticdc artifacts starting from v8.5.4.
Critical Issues
-
Inconsistent version comparison logic in
tag-rc2ga-on-repos.sh(lines ~118):# Old: if [[ "$(printf '%s\n' "v9.0.0" "$ga_ver" | sort -V | head -n1)" == "v9.0.0" ]]; then # New: if [[ "$(printf '%s\n' "v8.5.4" "$ga_ver" | sort -V | tail -n1)" == "$ga_ver" ]]; then
The two version comparison lines differ in logic and direction:
- At line 54, the check uses
head -n1to verify ifga_veris greater or equal to"v8.5.4". - At line 118, the check uses
tail -n1and compares to$ga_ver.
This inconsistency can cause subtle bugs where one condition triggers correctly and the other does not.
Suggestion:
Standardize version comparison logic to avoid confusion and ensure correctness. For example:
# Checks if ga_ver >= v8.5.4 if [[ "$(printf '%s\n' "v8.5.4" "$ga_ver" | sort -V | head -n1)" == "v8.5.4" ]]; then # logic here fi
Apply the same pattern consistently in both places.
- At line 54, the check uses
Code Improvements
-
Use explicit version comparison utilities or functions:
The scripts rely on
printf+sort -V+head/tailfor version comparison, which is error-prone and hard to read.Consider extracting this into a reusable function for clarity and maintainability, e.g.:
version_gte() { # returns 0 if $1 >= $2, 1 otherwise [ "$(printf '%s\n' "$2" "$1" | sort -V | head -n1)" = "$2" ] } if version_gte "$ga_ver" "v8.5.4"; then # ... fi
This reduces duplication and clarifies intent.
-
TypeScript version comparison by string comparison:
In
check-images-internal.tsandcheck-tiup.ts, version comparisons likeversion <= "v8.5.3"rely on lexicographic string comparison, which can be unreliable for semantic versions.Suggestion: Use a proper semantic version comparison library or parse versions to numeric components before comparison to avoid edge cases (e.g.
"v8.10.0"vs"v8.5.4").Example improvement:
import semver from 'semver'; if (semver.lte(version.replace(/^v/, ''), '8.5.3')) { // ... }
-
Permissions change in
collect-info-for-release-issue.sh:Mode changed from 644 to 755 (executable). Confirm this is intentional and consistent with how this script is invoked.
Best Practices
-
Documentation and comments:
-
Some comments have typos or could be clearer, e.g.:
// ticdc is initilized since v8.5.4"initilized" → "initialized"
-
Comments mentioning version thresholds should consistently reflect current logic.
-
-
Testing coverage:
- No evidence of tests or instructions about verifying these version threshold changes.
Suggestion: Add or update automated tests validating version gating behavior for
pingcap/ticdcatv8.5.4. -
Style consistency:
-
In Bash, sometimes conditionals use
[[ ... ]]with different patterns. -
Consider consistent quoting of variables in Bash to avoid word splitting, e.g.:
if [[ "$(printf '%s\n' "v8.5.4" "$ga_ver" | sort -V | head -n1)" == "v8.5.4" ]]; then
is good, but ensure
$ga_veris always quoted.
-
Summary of Actionable Items
- Standardize version comparison logic in
tag-rc2ga-on-repos.shfor consistency. - Extract version comparison to reusable Bash functions.
- Use proper semantic version comparison in TypeScript scripts (consider
semver). - Fix typo in comments (
initilized→initialized). - Verify permission change on
collect-info-for-release-issue.sh. - Add or update tests verifying the version gating behavior.
- Ensure consistent quoting and style in shell scripts.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR updates the version threshold for handling the pingcap/ticdc repository in several release-related scripts, shifting it from v9.0.0 down to v8.5.4. The changes span shell scripts and TypeScript files, affecting version gating logic and release version variables. The implementation is straightforward and consistent across scripts. Overall, the changes are clear and focused, but there are some inconsistencies and potential logic errors in version comparisons that need addressing.
Critical Issues
-
Incorrect version comparison logic in
tag-rc2ga-on-repos.sh(lines 54 and 119):
The two shell conditionals changed from:if [[ "$(printf '%s\n' "v9.0.0" "$ga_ver" | sort -V | head -n1)" == "v9.0.0" ]]; then
to
if [[ "$(printf '%s\n' "v8.5.4" "$ga_ver" | sort -V | tail -n1)" == "$ga_ver" ]]; then
This changes the logic from "ga_ver >= threshold" (version >=
v9.0.0) to "ga_ver <= threshold" (version <=v8.5.4), which is opposite to the intended gating (should run if version is at leastv8.5.4). This introduces a bug wherepingcap/ticdcwill be included only for versions less than or equal tov8.5.4, not greater or equal.Suggested fix: Use consistent version comparison, for example:
if [[ "$(printf '%s\n' "v8.5.4" "$ga_ver" | sort -V | head -n1)" == "v8.5.4" ]]; then
This checks if
ga_veris ≥v8.5.4.
Code Improvements
-
Inconsistent version comparison style across scripts:
- In shell scripts (
tag-rc2ga-on-repos.sh), version comparisons are done viasort -V. - In TypeScript files (
check-images-internal.ts,check-tiup.ts), comparisons are done via string comparison operators (>=,<=).
While this may work due to the format
vX.Y.Z, it is safer and more robust to use a proper semantic version comparison library or function in TypeScript to avoid edge cases and ensure correctness.Suggestion:
Implement or import a semver comparison helper in TypeScript instead of relying on string comparison. - In shell scripts (
-
Off-by-one errors in version boundary checks:
check-images-internal.tsskipspingcap/ticdcfor versions<= v8.5.3, meaning fromv8.5.4onwards it is included, which matches the PR intent.check-tiup.tsuses<= v8.5.4to skip, which excludesv8.5.4itself, contradicting the intention to include fromv8.5.4.
Suggestion:
Align the boundary logic so thatv8.5.4is included, e.g., use< v8.5.4or<= v8.5.3consistently.
Best Practices
-
Documentation clarity:
- The comment in
check-tiup.ts:is misleading because it excludes// ticdc is initilized since v8.5.4 if (version <= "v8.5.4" && ociRepo === "pingcap/ticdc/package") { continue; }
v8.5.4itself.
Suggestion:
Clarify comments and ensure they match the code precisely, e.g.:// Exclude ticdc for versions before v8.5.4 (exclusive) if (version < "v8.5.4" && ociRepo === "pingcap/ticdc/package") { continue; }
- The comment in
-
Testing coverage:
There is no indication of added or updated tests verifying this version threshold change.Suggestion:
Add or update test cases (unit or integration) covering version gating logic forpingcap/ticdcaround thev8.5.4boundary to prevent regressions. -
Script executable flag:
collect-info-for-release-issue.shchanged from mode 644 to 755, presumably to make it executable. This is good but should be documented or noted explicitly in the PR description for clarity.
Summary of Action Items
| File | Line(s) | Issue | Suggested Fix |
|---|---|---|---|
tag-rc2ga-on-repos.sh |
54, 119 | Version comparison logic inverted | Use head -n1 instead of tail -n1 to check ga_ver >= v8.5.4 |
scripts/flow/rc/check-tiup.ts |
223-224 | Boundary includes v8.5.4 incorrectly excluded |
Change <= "v8.5.4" to < "v8.5.4" or adjust comment accordingly |
scripts/flow/rc/check-images-internal.ts |
158 | Use of string comparison for versions | Consider using semver comparison helper |
scripts/flow/rc/check-tiup.ts |
220-225 | Comments do not clearly explain version boundaries | Update comments to precisely describe version gating |
| Various | N/A | No test coverage for updated version gating | Add or update tests to cover pingcap/ticdc version threshold changes |
Addressing the critical version comparison logic errors is the highest priority before merging. After that, improve consistency, documentation, and testing for robust maintainability.
|
/approve |
|
[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 |
Change comparison from <= to < so v8.5.4 is treated as initialized
[LGTM Timeline notifier]Timeline:
|
|
New changes are detected. LGTM label has been removed. |
This pull request updates the version gating logic for the
pingcap/ticdcrepository across several release and artifact management scripts. The main change is shifting the version threshold for including or initializingpingcap/ticdcfromv9.0.0down tov8.5.4, ensuring that relevant scripts and processes now handlepingcap/ticdcstarting from versionv8.5.4instead ofv9.0.0.Version threshold updates for
pingcap/ticdc:tag-oci-artifact-repos.shandtag-oci-image-repos.shto includepingcap/ticdcstarting fromv8.5.4instead ofv9.0.0. [1] [2]collect-info-for-release-issue.shto reflect thatpingcap/ticdcis included sincev8.5.4.pingcap/ticdcincheck-images-internal.tsandcheck-tiup.tsto usev8.5.4as the threshold. [1] [2]Release version updates:
previous_releaseandcurrent_releasevariables incollect-info-for-release-issue.shtov8.5.3andv8.5.4respectively.