version: impl the tidb-x version schema#66659
version: impl the tidb-x version schema#66659D3Hunter wants to merge 14 commits intopingcap:masterfrom
Conversation
|
Review Complete Findings: 3 issues ℹ️ Learn more details on Pantheon AI. |
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes version initialization, adds NextGen (TiDB‑X) release-version parsing/normalization and server-version composition, updates display logic and tests to be kernel-aware, and adjusts build/test dependencies and module requirements for semver handling. Changes
Sequence DiagramsequenceDiagram
participant Server as Server Init
participant Config as Config
participant Kernel as Kernel Detector
participant Parser as Version Parser
participant Display as Display Logic
Server->>Config: load cfg
Server->>Parser: initVersions(cfg)
Parser->>Kernel: IsNextGen()
alt NextGen
Kernel-->>Parser: true
Parser->>Parser: NormalizeTiDBReleaseVersionForNextGen(cfg.TiDBReleaseVersion)
Parser->>Parser: BuildTiDBXReleaseVersion(normalized)
Parser->>Parser: BuildTiDBXServerVersion(tidbXRelease)
Parser-->>Server: set ServerVersion & TiDBReleaseVersion & TiDBEdition
else Classic
Kernel-->>Parser: false
Parser->>Parser: apply cfg.ServerVersion / cfg.TiDBReleaseVersion
Parser-->>Server: set ServerVersion & TiDBReleaseVersion & TiDBEdition
end
Server->>Display: PrintTiDBInfo() / GetTiDBInfo()
Display->>Kernel: IsNextGen()
Display->>Parser: getReleaseVersionsForDisplay()
Display-->>Server: log/display Release Version (+ Component Version if NextGen)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/parser/mysql/const.go (1)
41-51: Avoid placeholder literal drift by reusing the constant.
legacyTiDBReleaseVersionPlaceholderandTiDBReleaseVersioncurrently duplicate the same literal. Use the constant for initialization so future updates cannot diverge.♻️ Suggested change
- TiDBReleaseVersion = "v8.4.0-this-is-a-placeholder" + TiDBReleaseVersion = legacyTiDBReleaseVersionPlaceholder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/mysql/const.go` around lines 41 - 51, TiDBReleaseVersion currently duplicates the literal used in legacyTiDBReleaseVersionPlaceholder; change the initialization of TiDBReleaseVersion to reference legacyTiDBReleaseVersionPlaceholder (e.g., TiDBReleaseVersion = legacyTiDBReleaseVersionPlaceholder) so the placeholder literal is defined in one place and cannot drift between the two symbols.pkg/parser/mysql/const_test.go (1)
120-120: Prefer the placeholder constant over a duplicated string literal.Using the constant keeps this test resilient if the placeholder text changes.
🧹 Suggested change
- require.Equal(t, TiDBXPlaceholderReleaseVersion, NormalizeTiDBReleaseVersionForNextGen("v8.4.0-this-is-a-placeholder")) + require.Equal(t, TiDBXPlaceholderReleaseVersion, NormalizeTiDBReleaseVersionForNextGen(legacyTiDBReleaseVersionPlaceholder))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/mysql/const_test.go` at line 120, Replace the duplicated placeholder string literal with the existing constant to avoid brittle tests: update the assertion so NormalizeTiDBReleaseVersionForNextGen is called with TiDBXPlaceholderReleaseVersion instead of the hard-coded "v8.4.0-this-is-a-placeholder"; this touches the test assertion using NormalizeTiDBReleaseVersionForNextGen and TiDBXPlaceholderReleaseVersion in const_test.go.pkg/util/printer/printer.go (1)
45-49: Log conversion fallback errors for observability.The fallback is reasonable, but swallowing the error makes diagnosis harder in non-standard flows. Emit a warning before returning.
🔎 Suggested change
tidbXReleaseVersion, err := mysql.BuildTiDBXReleaseVersion(normalizedReleaseVersion) if err != nil { // Startup validates this value in nextgen. Keep this fallback to avoid // panics when helper is called outside the normal startup flow. + logutil.BgLogger().Warn("failed to build nextgen release version, falling back to raw release version", + zap.String("releaseVersion", releaseVersion), + zap.String("normalizedReleaseVersion", normalizedReleaseVersion), + zap.Error(err), + ) return releaseVersion, "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/printer/printer.go` around lines 45 - 49, In the error fallback branch where it currently returns releaseVersion, "" on err != nil, emit a warning including the actual err before returning to improve observability; update the if err != nil block to log a concise warning (using the package's logger or std log) that includes err and that you're falling back to releaseVersion, then return releaseVersion, "" as before.cmd/tidb-server/main_test.go (1)
176-183: Derive expected server version in-test instead of hardcoding it.Line 182 hardcodes
"8.0.11-TiDB-X-CLOUD.202603.0". Building the expected value via helper APIs makes this test resilient to future canonical-version constant updates.♻️ Suggested test refactor
require.NoError(t, setVersionByConfig(config.GetGlobalConfig())) require.Equal(t, mysql.TiDBXPlaceholderReleaseVersion, mysql.TiDBReleaseVersion) - require.Equal(t, "8.0.11-TiDB-X-CLOUD.202603.0", mysql.ServerVersion) + expectedServerVersion, err := mysql.BuildTiDBXServerVersion(mysql.TiDBXPlaceholderReleaseVersion) + require.NoError(t, err) + require.Equal(t, expectedServerVersion, mysql.ServerVersion) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/tidb-server/main_test.go` around lines 176 - 183, The test currently hardcodes the expected ServerVersion string; instead compute it from the configured release so the test stays correct when canonical version logic changes: after calling setVersionByConfig(config.GetGlobalConfig()), derive the expected value using the package helper that composes server versions from a release (e.g. the mysql helper used by production code that builds ServerVersion from TiDBReleaseVersion) and assert require.Equal(t, expected, mysql.ServerVersion) while still asserting mysql.TiDBReleaseVersion == mysql.TiDBXPlaceholderReleaseVersion; update references around setVersionByConfig, mysql.TiDBReleaseVersion, and mysql.ServerVersion accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/tidb-server/main.go`:
- Around line 756-768: The function setVersionByConfig mutates globals
(versioninfo.TiDBEdition and mysql.TiDBReleaseVersion) before performing NextGen
validation, causing partial global updates if BuildTiDBXServerVersion fails;
change the logic to use local variables (e.g., newEdition, newReleaseVersion)
and perform NormalizeTiDBReleaseVersionForNextGen and BuildTiDBXServerVersion
validation against those locals inside the kerneltype.IsNextGen branch, and only
if validation succeeds assign the locals back to versioninfo.TiDBEdition and
mysql.TiDBReleaseVersion (and return any error without leaving globals mutated).
---
Nitpick comments:
In `@cmd/tidb-server/main_test.go`:
- Around line 176-183: The test currently hardcodes the expected ServerVersion
string; instead compute it from the configured release so the test stays correct
when canonical version logic changes: after calling
setVersionByConfig(config.GetGlobalConfig()), derive the expected value using
the package helper that composes server versions from a release (e.g. the mysql
helper used by production code that builds ServerVersion from
TiDBReleaseVersion) and assert require.Equal(t, expected, mysql.ServerVersion)
while still asserting mysql.TiDBReleaseVersion ==
mysql.TiDBXPlaceholderReleaseVersion; update references around
setVersionByConfig, mysql.TiDBReleaseVersion, and mysql.ServerVersion
accordingly.
In `@pkg/parser/mysql/const_test.go`:
- Line 120: Replace the duplicated placeholder string literal with the existing
constant to avoid brittle tests: update the assertion so
NormalizeTiDBReleaseVersionForNextGen is called with
TiDBXPlaceholderReleaseVersion instead of the hard-coded
"v8.4.0-this-is-a-placeholder"; this touches the test assertion using
NormalizeTiDBReleaseVersionForNextGen and TiDBXPlaceholderReleaseVersion in
const_test.go.
In `@pkg/parser/mysql/const.go`:
- Around line 41-51: TiDBReleaseVersion currently duplicates the literal used in
legacyTiDBReleaseVersionPlaceholder; change the initialization of
TiDBReleaseVersion to reference legacyTiDBReleaseVersionPlaceholder (e.g.,
TiDBReleaseVersion = legacyTiDBReleaseVersionPlaceholder) so the placeholder
literal is defined in one place and cannot drift between the two symbols.
In `@pkg/util/printer/printer.go`:
- Around line 45-49: In the error fallback branch where it currently returns
releaseVersion, "" on err != nil, emit a warning including the actual err before
returning to improve observability; update the if err != nil block to log a
concise warning (using the package's logger or std log) that includes err and
that you're falling back to releaseVersion, then return releaseVersion, "" as
before.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/tidb-server/BUILD.bazelcmd/tidb-server/main.gocmd/tidb-server/main_test.gopkg/expression/integration_test/BUILD.bazelpkg/expression/integration_test/integration_test.gopkg/parser/mysql/BUILD.bazelpkg/parser/mysql/const.gopkg/parser/mysql/const_test.gopkg/util/printer/BUILD.bazelpkg/util/printer/printer.gopkg/util/printer/printer_test.go
D3Hunter
left a comment
There was a problem hiding this comment.
AI-generated review based on @D3Hunter's standards; manual review will be performed after all comments are resolved.
Summary
- Total findings: 8
- Inline comments: 7
- Summary-only findings (no inline anchor): 1
Findings (highest risk first)
🚨 [Blocker] (1)
- Nextgen startup rejects common build-injected TiDB release strings (
cmd/tidb-server/main.go:764, cmd/tidb-server/main.go:781, cmd/tidb-server/main.go:879, pkg/parser/mysql/const.go:55, Makefile.common:83, build/print-workspace-status.sh:25)
⚠️ [Major] (4)
Release Versionlabel carries different semantics across kernel types (pkg/util/printer/printer.go:57; pkg/expression/integration_test/integration_test.go:1673; pkg/expression/integration_test/integration_test.go:1676)- Kernel-conditional version logic is not fully reflected in existing global-vars test path (
cmd/tidb-server/main.go:763, cmd/tidb-server/main.go:880, cmd/tidb-server/main_test.go:77) - Nextgen server-version string is not compatible with DDL job-version semver parsing (
pkg/parser/mysql/const.go:87) server-versionconfig is silently ignored on nextgen code path (cmd/tidb-server/main.go:763)
🟡 [Minor] (3)
TiDBXReleaseVersionPrefixis exported without external consumers (pkg/parser/mysql/const.go:39)- Legacy placeholder string is duplicated in same file instead of using one canonical definition (
pkg/parser/mysql/const.go:41, pkg/parser/mysql/const.go:50) - No regression test covers DDL mixed-version detection with
TiDB-X-CLOUDserver-version (DDL mixed-version detection path in pkg/ddl (no direct diff anchor in this PR))
Unanchored findings
🟡 [Minor] (1)
- No regression test covers DDL mixed-version detection with
TiDB-X-CLOUDserver-version- Scope:
DDL mixed-version detection path in pkg/ddl (no direct diff anchor in this PR) - Request: add UT for it: include a mocked server-info case with
8.0.11-TiDB-X-CLOUD.202603.0and assert the intendedJobVersionbehavior.
- Scope:
Codecov Report❌ Patch coverage is Please upload reports for the commit 060c9f7 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #66659 +/- ##
================================================
+ Coverage 77.6714% 78.3551% +0.6837%
================================================
Files 2008 1945 -63
Lines 549230 538050 -11180
================================================
- Hits 426595 421590 -5005
+ Misses 120964 115470 -5494
+ Partials 1671 990 -681
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/util/printer/printer.go (1)
83-110:⚠️ Potential issue | 🟠 Major
GetTiDBInfo()discards the component version needed for NextGentidb_version()output.Line 83 discards the second return value from
getReleaseVersionsForDisplay(), so theTiDB Component Versionfield is never added to the output.PrintTiDBInfo()correctly captures and includes this field (lines 66–67), butGetTiDBInfo()does not. Thetidb_version()SQL function relies onGetTiDBInfo()(seepkg/expression/builtin_info.go:592), and integration tests expect this field to be present for NextGen builds (seepkg/expression/integration_test/integration_test.go:1674).Proposed fix
func GetTiDBInfo() string { - releaseVersion, _ := getReleaseVersionsForDisplay() + releaseVersion, componentVersion := getReleaseVersionsForDisplay() + componentVersionLine := "" + if componentVersion != "" { + componentVersionLine = fmt.Sprintf("TiDB Component Version: %s\n", componentVersion) + } enterpriseVersion := "" if versioninfo.TiDBEnterpriseExtensionGitHash != "" { enterpriseVersion = fmt.Sprintf("\nEnterprise Extension Commit Hash: %s", versioninfo.TiDBEnterpriseExtensionGitHash) } info := fmt.Sprintf("Release Version: %s\n"+ + "%s"+ "Edition: %s\n"+ "Git Commit Hash: %s\n"+ "Git Branch: %s\n"+ "UTC Build Time: %s\n"+ "GoVersion: %s\n"+ "Race Enabled: %v\n"+ "Check Table Before Drop: %v\n"+ "Store: %s"+ "%s", releaseVersion, + componentVersionLine, versioninfo.TiDBEdition, versioninfo.TiDBGitHash, versioninfo.TiDBGitBranch, versioninfo.TiDBBuildTS, buildVersion,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/printer/printer.go` around lines 83 - 110, GetTiDBInfo() currently discards the second return value from getReleaseVersionsForDisplay(), which omits the "TiDB Component Version" field relied on by tidb_version(); modify GetTiDBInfo() to capture both return values (e.g., releaseVersion and componentVersion) from getReleaseVersionsForDisplay() and add the componentVersion into the formatted info string (same label used by PrintTiDBInfo()), ensuring the component version is appended before returning the info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/tidb-server/main_test.go`:
- Around line 97-183: The tests only exercise initVersions unit behavior; add an
integration test that actually starts the tidb-server entrypoint and verifies
startup and basic SQL functionality after changing the config-based version
values. Create a new integration test that (1) uses the same config updates as
TestSetVersionByConfig (set conf.ServerVersion / conf.TiDBReleaseVersion /
conf.TiDBEdition), (2) launches the tidb-server entrypoint (the main/start
routine used by cmd/tidb-server) as a subprocess or via the integration harness,
(3) waits for readiness, executes a simple query (e.g., SELECT 1) against the
server to confirm it is serving, and (4) validates the effective version values
(mysql.ServerVersion, mysql.TiDBReleaseVersion or via SHOW VARIABLES/SELECT
version()) match expectations; finally ensure proper cleanup/kill of the process
and restore config. Use initVersions, TestSetVersionByConfig inputs as reference
for the version values and cleanup logic.
In `@pkg/parser/mysql/const.go`:
- Around line 67-79: BuildTiDBXReleaseVersion currently accepts versions with
prerelease/metadata suffixes (e.g., v26.3.0-xxx); after trimming the leading "v"
and parsing with semver.NewVersion, add a check that ver.Prerelease() and
ver.Metadata() are empty (or equivalently reject rawVer containing '-' or '+')
and return the same formatted error if either is present so only exact vYY.M.F
(no suffixes) is allowed; reference the function BuildTiDBXReleaseVersion and
the variables releaseVersion, rawVer and ver to locate where to add this
validation.
---
Outside diff comments:
In `@pkg/util/printer/printer.go`:
- Around line 83-110: GetTiDBInfo() currently discards the second return value
from getReleaseVersionsForDisplay(), which omits the "TiDB Component Version"
field relied on by tidb_version(); modify GetTiDBInfo() to capture both return
values (e.g., releaseVersion and componentVersion) from
getReleaseVersionsForDisplay() and add the componentVersion into the formatted
info string (same label used by PrintTiDBInfo()), ensuring the component version
is appended before returning the info.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/parser/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/tidb-server/main.gocmd/tidb-server/main_test.gogo.modpkg/parser/go.modpkg/parser/mysql/BUILD.bazelpkg/parser/mysql/const.gopkg/parser/mysql/const_test.gopkg/util/printer/BUILD.bazelpkg/util/printer/printer.gopkg/util/printer/printer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/util/printer/BUILD.bazel
|
Review of commit 2dcdd6a could not be completed — environment setup failed after 3 attempts due to upstream infrastructure overload (503). Please re-trigger the review when the service recovers. ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/parser/mysql/const.go (1)
71-79:⚠️ Potential issue | 🟠 MajorEnforce exact
vYY.M.fixby rejecting prerelease/metadata suffixes.
semver.NewVersionon Line 71 accepts suffixes, so values likev26.3.0-rc1still pass current checks. If the schema is strict, this should be rejected explicitly in this function.Suggested fix
func BuildTiDBXReleaseVersion(releaseVersion string) (string, error) { if !strings.HasPrefix(releaseVersion, "v") { return "", errors.Errorf("invalid TiDB release version %q, should start with 'v'", releaseVersion) } rawVer := strings.TrimPrefix(releaseVersion, "v") + if strings.ContainsAny(rawVer, "-+") { + return "", errors.Errorf("invalid TiDB release version %q, expected format v[2-digit-year].[month].[fix-version]", releaseVersion) + } ver, err := semver.NewVersion(rawVer) if err != nil { return "", errors.Errorf("invalid TiDB release version %q, expect a semantic version", releaseVersion) } // our first release of next-gen since 2025 if ver.Major < 25 || ver.Major > 99 || ver.Minor < 1 || ver.Minor > 12 { - return "", errors.Errorf("invalid TiDB release version %q, the semantic version part should be in [2-digit-year].[month].[fix-version]-[xxx] format", releaseVersion) + return "", errors.Errorf("invalid TiDB release version %q, expected format v[2-digit-year].[month].[fix-version]", releaseVersion) } return fmt.Sprintf("%s20%02d%02d.%d", tidbXReleaseVersionPrefix, ver.Major, ver.Minor, ver.Patch), nil }Run this to verify the current code path doesn’t explicitly reject suffixes and whether tests cover them:
#!/bin/bash set -euo pipefail rg -n -C2 'func BuildTiDBXReleaseVersion|semver.NewVersion|ContainsAny|PreRelease|Metadata' pkg/parser/mysql/const.go rg -n -C2 'BuildTiDBXReleaseVersion|v[0-9]{2}\.[0-9]{1,2}\.[0-9]+-[A-Za-z0-9]|v[0-9]{2}\.[0-9]{1,2}\.[0-9]+\+[A-Za-z0-9]' pkg/parser/mysql/const_test.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/mysql/const.go` around lines 71 - 79, The parsing currently accepts semver suffixes like prerelease/metadata; update BuildTiDBXReleaseVersion to explicitly reject versions with prerelease or metadata by checking ver.Prerelease() and ver.Metadata() (or equivalent methods on the semver Version) right after semver.NewVersion(rawVer) and returning an error that the releaseVersion must be exact vYY.M.fix without suffixes; reference the variables ver, rawVer and releaseVersion in the new check and ensure the error message clearly states that prerelease/metadata suffixes are not allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile.common`:
- Around line 47-50: The default TIDB_X_RELEASE_VERSION currently appends a
git-describe suffix making it `vYY.M.0-<git-describe>` which violates the strict
vYY.M.fix schema; change the default assignment of TIDB_X_RELEASE_VERSION so it
produces only the semantic `vYY.M.0` format (e.g., compose v$(shell date
'+%y').$(shell date '+%m' | sed 's/^0//').0) and remove the appended -$(shell
git describe --always --dirty --exclude '*') suffix, leaving
TIDB_RELEASE_VERSION := $(TIDB_X_RELEASE_VERSION) unchanged so overrides via
environment or explicit git-based values still work when provided.
---
Duplicate comments:
In `@pkg/parser/mysql/const.go`:
- Around line 71-79: The parsing currently accepts semver suffixes like
prerelease/metadata; update BuildTiDBXReleaseVersion to explicitly reject
versions with prerelease or metadata by checking ver.Prerelease() and
ver.Metadata() (or equivalent methods on the semver Version) right after
semver.NewVersion(rawVer) and returning an error that the releaseVersion must be
exact vYY.M.fix without suffixes; reference the variables ver, rawVer and
releaseVersion in the new check and ensure the error message clearly states that
prerelease/metadata suffixes are not allowed.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile.commonpkg/expression/integration_test/integration_test.gopkg/parser/mysql/const.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/expression/integration_test/integration_test.go
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/parser/mysql/const.go (1)
72-79:⚠️ Potential issue | 🟡 MinorReject
+buildmetadata to keep the schema strict.Line 72 relies on
semver.NewVersion, which accepts build metadata (+...). That permits inputs likev26.3.0+meta, while the declared schema isvYY.M.F[-suffix].Suggested fix
ver, err := semver.NewVersion(rawVer) if err != nil { return "", errors.Errorf("invalid TiDB release version %q, expect a semantic version", releaseVersion) } + if ver.Metadata != "" { + return "", errors.Errorf("invalid TiDB release version %q, the semantic version part should be in [2-digit-year].[month].[fix-version]-[xxx] format", releaseVersion) + } // our first release of next-gen since 2025 if ver.Major < 25 || ver.Major > 99 || ver.Minor < 1 || ver.Minor > 12 { return "", errors.Errorf("invalid TiDB release version %q, the semantic version part should be in [2-digit-year].[month].[fix-version]-[xxx] format", releaseVersion) }In github.com/coreos/go-semver v0.3.1, does semver.NewVersion("26.3.0+meta") parse successfully and set the Metadata field?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/mysql/const.go` around lines 72 - 79, The semver.NewVersion call currently allows build metadata (e.g., "v26.3.0+meta") which violates the required schema; after parsing (the ver variable returned from semver.NewVersion) add a check that ver.Metadata is empty and, if not, return an error (use the same error formatting with releaseVersion) so any version containing +build metadata is rejected; reference semver.NewVersion, ver.Metadata and releaseVersion when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@br/pkg/version/version.go`:
- Around line 413-414: The year validation currently accepts 2000–2099 using the
condition "if err != nil || year < 2000 || year > 2099 { return \"\" }" which is
too permissive; update that check in br/pkg/version/version.go to enforce the
producer constraint by changing the lower bound from 2000 to 2025 (i.e., use
"year < 2025" so the accepted range is 2025–2099), leaving the error check and
upper bound unchanged.
In `@Makefile`:
- Around line 735-743: The two bazel invocations that currently prefix with
NEXT_GEN=$(NEXT_GEN) (the commands invoking bazel to build
//cmd/importer:importer //cmd/tidb-server:tidb-server ... and the later
//cmd/tidb-server:tidb-server enterprise build) must include
--action_env=NEXT_GEN so the workspace status command receives the NEXT_GEN
environment variable; update both bazel command lines in the Makefile to add
--action_env=NEXT_GEN to their argument lists while keeping the existing
--workspace_status_command and other defines intact.
- Around line 725-726: The Makefile's bazel invocation sets NEXT_GEN in the
shell but the workspace status command
(workspace_status_command=./build/print-workspace-status.sh which calls
./build/compute-tidb-release-version.sh) won't see that because of
--incompatible_strict_action_env; update the bazel command that builds
//cmd/importer:importer and //cmd/tidb-server:tidb-server to pass NEXT_GEN into
the action environment by adding --action_env=NEXT_GEN so the workspace status
script can read NEXT_GEN.
---
Duplicate comments:
In `@pkg/parser/mysql/const.go`:
- Around line 72-79: The semver.NewVersion call currently allows build metadata
(e.g., "v26.3.0+meta") which violates the required schema; after parsing (the
ver variable returned from semver.NewVersion) add a check that ver.Metadata is
empty and, if not, return an error (use the same error formatting with
releaseVersion) so any version containing +build metadata is rejected; reference
semver.NewVersion, ver.Metadata and releaseVersion when making the change.
ℹ️ Review info
Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f36725a-31c6-4503-bbc2-007d4d1cd31b
📒 Files selected for processing (9)
MakefileMakefile.commonbr/pkg/version/BUILD.bazelbr/pkg/version/version.gobr/pkg/version/version_test.gobuild/compute-tidb-release-version.shbuild/print-enterprise-workspace-status.shbuild/print-workspace-status.shpkg/parser/mysql/const.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile.common
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
D3Hunter
left a comment
There was a problem hiding this comment.
AI-generated review based on @D3Hunter's standards; manual review will be performed after all comments are resolved.
Summary
- Total findings: 12
- Inline comments: 12
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (8)
- NextGen release-policy constants are exported without a clear external contract (
pkg/parser/mysql/const.go:44) - BR server-version parsing depends on local kernel build and downgrades opposite formats to 0.0.0 (
br/pkg/version/version.go:450) - Nextgen init path silently ignores configured version fields instead of enforcing a clear policy (
cmd/tidb-server/main.go:759) - Nextgen startup path ignores configured version fields and skips explicit misconfiguration handling (
cmd/tidb-server/main.go:759) - BR server-version parsing is build-kernel-dependent and collapses unsupported formats to 0.0.0 (
br/pkg/version/version.go:449) - NextGen release-version rules are duplicated across shell and Go layers (
build/compute-tidb-release-version.sh:41; pkg/parser/mysql/const.go:83; br/pkg/version/version.go:414) - NextGen startup silently ignores version-related config keys (
cmd/tidb-server/main.go:759) - BR version parsing is gated by local kernel mode and falls back to 0.0.0 on format mismatch (
br/pkg/version/version.go:450)
🟡 [Minor] (3)
parseTiDBXVersionname hides a semantic conversion contract (br/pkg/version/version.go:404)TIDB_X_RELEASE_VERSIONoverride is accepted without format validation (build/compute-tidb-release-version.sh:33)- Build-time override
TIDB_X_RELEASE_VERSIONis accepted without format validation (build/compute-tidb-release-version.sh:33)
🧹 [Nit] (1)
- NextGen version-init branch comment is unclear and contains a typo (
cmd/tidb-server/main.go:757)
|
Review Complete Findings: 3 issues ℹ️ Learn more details on Pantheon AI. |
D3Hunter
left a comment
There was a problem hiding this comment.
AI-generated review based on @D3Hunter's standards; manual review will be performed after all comments are resolved.
Summary
- Total findings: 10
- Inline comments: 9
- Summary-only findings (no inline anchor): 1
Findings (highest risk first)
⚠️ [Major] (6)
- Nextgen server version format breaks DDL semver parsing (
DDL job-version detection path for server version parsing (non-diff file)) - Nextgen startup can panic on legacy version overrides instead of failing with controlled config validation (
cmd/tidb-server/main.go:759) - ParseServerInfo now hides format parsing behind build-tag mode, not input format (
br/pkg/version/version.go:450) initVersionsmixes policy validation and version derivation, creating an unclear abstraction boundary (cmd/tidb-server/main.go:756)- Nextgen now hard-fails on legacy version override config keys without a compatibility transition (
cmd/tidb-server/main.go:759) - Nextgen version-init behavior is not deterministically validated in tests (
cmd/tidb-server/main.go:759)
🟡 [Minor] (3)
- NextGen release-version naming drifts between
TiDBXandtidbxfor the same concept (pkg/parser/mysql/const.go:37) - Nextgen fallback release version is timezone-dependent for identical source revisions (
build/compute-tidb-release-version.sh:44) - Date-based nextgen release synthesis is environment-dependent (
build/compute-tidb-release-version.sh:44)
🧹 [Nit] (1)
- Policy comments around version override restrictions explain mechanics but not intent (
cmd/tidb-server/main.go:757)
Unanchored findings
⚠️ [Major] (1)
- Nextgen server version format breaks DDL semver parsing
- Scope:
DDL job-version detection path for server version parsing (non-diff file) - Request: Normalize nextgen server-version suffix to semver before semver.NewVersion in DDL detection, or add a dedicated nextgen parsing branch that maps TiDB-X-CLOUD.. to a comparable semantic version.
- Scope:
|
Review of commit 33556f4 could not complete — environment preparation failed after 3 attempts due to infrastructure unavailability (upstream provider overloaded). Will retry automatically when the next commit is pushed, or can be re-triggered manually. ℹ️ Learn more details on Pantheon AI. |
|
⏳ @D3Hunter I've received your updated pull request and will continue the review. I'll update this comment when I have something to share. ℹ️ Learn more details on Pantheon AI. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn 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. |
|
@D3Hunter: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
⏳ @D3Hunter I've received your updated pull request and will continue the review. I'll update this comment when I have something to share. ℹ️ Learn more details on Pantheon AI. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #66657
Problem Summary:
What changed and how does it work?
build/compute-tidb-release-version.shto unify release-version derivation; wired it into workspace status scripts and Makefile/Bazel (NEXT_GENenv propagation).tidb-edition/tidb-release-version/server-versionconfig overrides and derives runtime versions from build info.v<YY>.<M>.<patch>and convert toTiDB-X-CLOUD.<YYYYMM>.<patch>.TiDB-X-CLOUDand convert it back to semver-like values for compatibility checks.Check List
Tests
result when run on Classic
result when run on nextgen
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Enhancements
Tests
Chores