Skip to content

fix(core): normalize partial-semver versions + drop dead e2e carve-out#476

Open
Dav-14 wants to merge 1 commit into
mainfrom
fix/version-validate-partial-semver-drop-e2e-carveout
Open

fix(core): normalize partial-semver versions + drop dead e2e carve-out#476
Dav-14 wants to merge 1 commit into
mainfrom
fix/version-validate-partial-semver-drop-e2e-carveout

Conversation

@Dav-14

@Dav-14 Dav-14 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

ValidateMinimumVersion("v3.2") silently returned nil because semver.IsValid("v3.2") is false (golang.org/x/mod/semver requires v<major>.<minor>.<patch>). The short-circuit at semver.IsValid(…) && Compare(…) < 0 meant every partial-semver Versions resource name (v3, v3.2, …) bypassed the minimum-version gate entirely. If MinimumStackVersion is ever bumped past such a name, the gate would silently let it through.

Fix

  • Add normalizePartialSemver which expands v3v3.0.0 and v3.2v3.2.0 so partial-semver names are gated by the same minimum as their canonical form. Non-matching inputs (canonical semver, dev tags, SHA refs, non-v-prefixed strings) are returned unchanged and continue to pass through.

Drive-by cleanups in the same function

  • if strings.TrimPrefix(version, "v") == "0.0.0-e2e" { return nil } → removed. It was dead. ValidateMinimumVersion has exactly one call site (in GetModuleVersion) that fires only when stack.Spec.VersionsFromFile is set AND both module.Version and stack.Spec.Version are empty. Every tests/e2e/chainsaw/**/resources/stack.yaml uses spec.version: v0.0.0-e2e, so the validator never sees the carve-out for them. semver already understands -e2e as a valid pre-release suffix anyway — a real e2e tag at or above the minimum sails through naturally.
  • if !strings.HasPrefix(version, "v") { version = "v" + version } → removed. golang.org/x/mod/semver contractually requires the v prefix; non-prefixed strings are exactly the "non-semver names allowed through" case the docstring already calls out. semver.IsValid returns false for them and they pass through — consistent and removes the only remaining branch in the function body.

Test plan

  • go test ./internal/core/... -run TestValidateMinimumVersion — 17/17 pass
  • go build ./internal/core/… clean
  • go vet ./internal/core/… clean

Test-table delta

Removed Why
2.1.0 rejected without v prefix Hinged on the now-removed prefix-injection branch
2.2.0 accepted without v prefix Same — replaced by explicit non-v-prefixed accepted as passthrough
v0.0.0-e2e accepted as test tag Carve-out gone; the call path never reaches this branch for chainsaw stacks
0.0.0-e2e accepted as test tag without v prefix Same
Added Why
v3 accepted (expands to v3.0.0) Pins the new partial-semver normalization happy path
v3.2 accepted (expands to v3.2.0) Pins the case that motivated this PR
v2.2 accepted as equal to minimum (expands to v2.2.0) Boundary at the minimum
v2 rejected (expands to v2.0.0) Pins below-minimum partial
v2.1 rejected (expands to v2.1.0) Pins below-minimum partial with minor
non-v-prefixed accepted as passthrough Makes the new contract explicit

`ValidateMinimumVersion("v3.2")` silently returned `nil` because
`semver.IsValid("v3.2")` is false (semver requires major.minor.patch).
The short-circuit at `semver.IsValid(...) && Compare(...) < 0` meant
every partial-semver Versions resource name (`v3`, `v3.2`, …) bypassed
the minimum-version gate entirely.

Adds `normalizePartialSemver` which expands `v3` → `v3.0.0` and
`v3.2` → `v3.2.0` before the comparison so partial-semver names are
gated by the same minimum as their canonical form. Non-matching
inputs (canonical semver, dev tags, SHA refs, non-`v`-prefixed
strings) are returned unchanged and continue to pass through.

Also drops two pieces of now-redundant code from the same function:

  - `if strings.TrimPrefix(version, "v") == "0.0.0-e2e" { return nil }`:
    a sentinel for e2e test builds. Dead: `ValidateMinimumVersion`
    has exactly one call site (inside `GetModuleVersion`) that fires
    only when `stack.Spec.VersionsFromFile` is set AND both
    `module.Version` and `stack.Spec.Version` are empty. Every
    chainsaw test stack uses `spec.version: v0.0.0-e2e`, so the
    validator is never invoked for them — the carve-out covered
    nothing. semver also already handles `-e2e` as a valid
    pre-release suffix; a real e2e tag at or above the minimum
    sails through naturally.

  - The `!strings.HasPrefix(version, "v")` dance: `golang.org/x/mod/semver`
    contractually requires the `v` prefix, and non-prefixed strings
    are exactly the "non-semver names allowed through" case the
    docstring already calls out. Letting them passthrough by way of
    `semver.IsValid` returning false is consistent and removes the
    only branch left in the function.

Tests:
  - Drops the two `0.0.0-e2e` cases (covered nothing — see above).
  - Drops the two non-`v`-prefixed cases that previously hinged on
    the prefix-injection branch (`2.1.0 rejected`, `2.2.0 accepted`);
    replaced by one explicit `non-v-prefixed accepted as passthrough`
    case making the intent visible.
  - Adds five partial-semver cases: `v3`, `v3.2`, `v2.2` accepted;
    `v2`, `v2.1` rejected.
@Dav-14 Dav-14 requested a review from a team as a code owner June 8, 2026 13:25
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 28144d83-4d9c-4945-be2e-fd28fa19c3cd

📥 Commits

Reviewing files that changed from the base of the PR and between b101556 and 3c1e3f2.

📒 Files selected for processing (2)
  • internal/core/version.go
  • internal/core/version_test.go

Walkthrough

This PR extends semver version validation to support partial version strings such as v3 and v3.2. A new normalization helper expands these into canonical forms (v3.0.0 and v3.2.0) before the minimum-version check, while non-semver inputs bypass the check entirely.

Changes

Partial Semver Version Normalization

Layer / File(s) Summary
Partial semver normalization utility
internal/core/version.go
Adds regexp import and introduces normalizePartialSemver helper with a regex pattern that converts partial v<major> or v<major>.<minor> strings to canonical v<major>.<minor>.0 form, leaving non-matching inputs unchanged.
ValidateMinimumVersion integration and tests
internal/core/version.go, internal/core/version_test.go
Rewrites ValidateMinimumVersion to normalize input versions via the helper and enforce the minimum-version check only when the normalized string is semver-valid. Test cases are updated to cover partial-semver expansion (e.g., v3, v3.2), canonical semver rejection/acceptance, and pass-through acceptance for non-semver inputs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • formancehq/operator#464: Also modifies internal/core/version.go's ValidateMinimumVersion flow and minimum-version enforcement logic, overlapping at the same version-validation code level.

Poem

🐰 Version strings, now flexible and free,
Partial semver expands naturally,
v3 becomes v3.0.0, just so,
Minimum checks dance through the flow!
Hops with delight through the test array. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: normalizing partial-semver versions and removing a dead e2e carve-out, directly matching the changeset.
Description check ✅ Passed The description comprehensively explains the bug being fixed, the solution implemented, drive-by cleanups, and test coverage—all directly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/version-validate-partial-semver-drop-e2e-carveout

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Dav-14 Dav-14 enabled auto-merge (squash) June 8, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant