Skip to content

ci: tighten the fully qualified version check (DRIVER-924) - #1049

Open
nikagra wants to merge 1 commit into
scylladb:scylla-4.xfrom
nikagra:fix/DRIVER-924-tighten-version-check
Open

ci: tighten the fully qualified version check (DRIVER-924)#1049
nikagra wants to merge 1 commit into
scylladb:scylla-4.xfrom
nikagra:fix/DRIVER-924-tighten-version-check

Conversation

@nikagra

@nikagra nikagra commented Sep 1, 2026

Copy link
Copy Markdown

The fully qualified version check added in #1043 could be bypassed, so a partial version could
still reach CCM — the one thing it exists to prevent. The cause was two predicates that disagreed:
a flag for freshly resolved values, a shape test for cached ones. This defines the grammar once and
applies it wherever a version is accepted.

  • A cached entry was rejected only when it was a bare MAJOR.MINOR, so a 4.1. or 4.1x left by
    an earlier Makefile was still served unchecked.
  • The exemption flag was never initialised, so an inherited SCYLLA_VERSION_EXACT=1 disabled the
    check. One shared grammar removes the flag and its double negative.
  • download-* and test-integration-* took the version from the environment behind only a
    non-empty test before handing it to ccm create / mvn — which is exactly how CI supplies it.
  • A warm cache returned before the GITHUB_OUTPUT write, leaving a step output empty.
  • Skip the scylla-enterprise lookup for OSS lines like 6.2, which never had releases there.

A version is exact when it is MAJOR.MINOR.PATCH, a pre-release label carrying its own number
(2022.2.0-rc0, 5.0.rc3, 4.0-alpha1), or a dated build id
(2024.2.3-0.20250108.931ce203dcf5, 5.4.0~dev-0.20230801.37b548f46365); all pass through with no
lookup. The number must sit in the label itself — CCM strips a trailing -x86_64/-aarch64 before
parsing, so 6.2.0-dev-aarch64 would otherwise reach it as the moving 6.2.0-dev.

Verified against the regex Make actually ships: 77 accept/reject cases covering every Cassandra and
Scylla form found in ccmlib, plus the invariant that no accepted version turns into a moving
selector once CCM strips the arch suffix. End to end with a stubbed get-version for the cache and
consumer cases — a poisoned entry evicted, a warm hit writing GITHUB_OUTPUT without refreshing the
mtime. Each case was re-run against the previous head to confirm it genuinely flipped. Not covered:
no integration suite was run locally, so the ccm create path rests on CI.

Follows #1043, which merged before these fixes were ready. The 3.x twin #1044 merged with the
earlier grammar; a port keeping the two byte-identical follows once this one is settled.
Refs: https://scylladb.atlassian.net/browse/DRIVER-924

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Team

Run ID: cb1ea691-7803-4038-b8a3-d5a09e62d34c

📥 Commits

Reviewing files that changed from the base of the PR and between bdda140 and 4cd82b9.

📒 Files selected for processing (1)
  • Makefile

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The Makefile now applies shared validation to fully qualified Cassandra and ScyllaDB versions. Resolvers reject invalid cache values, enforce exact pre-release formats, and write caches only after fresh resolution. ScyllaDB enterprise fallback applies only to four-digit-year versions. Download and integration-test targets validate versions before invoking CCM or Maven.

Sequence Diagram(s)

sequenceDiagram
  participant MakeTarget
  participant VersionResolver
  participant CCMOrMaven
  MakeTarget->>VersionResolver: resolve or receive server version
  VersionResolver-->>MakeTarget: return version value
  MakeTarget->>MakeTarget: validate fully qualified version
  MakeTarget->>CCMOrMaven: pass validated version
Loading

Merge Risk: ⚪ Minimal · up to 4cd82

The Makefile now consistently validates Cassandra and ScyllaDB version inputs, cache values, and resolved versions before CCM or Maven use. No concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
Title check ✅ Passed The title clearly identifies the main change: stricter fully qualified version validation.
Description check ✅ Passed The description directly explains the validation changes, affected workflows, edge cases, and verification scope.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Tightens server-version resolution to prevent partial or malformed versions from reaching CCM.

Changes:

  • Anchors suffixed-version validation and resets exemption flags.
  • Invalidates partial cached resolutions.
  • Avoids unnecessary ScyllaDB Enterprise lookups.
Suppressed comments (1)

Makefile:161

  • This only removes a cached two-component numeric value. The previous unanchored suffix branch could have written the other bypass examples (5.4. or 2026.2x), and these files remain cache hits that return before the tightened parser runs. Accept the cache only when its contents satisfy the current full-version or suffixed-version grammar.
	if [[ -f "${SCYLLA_VERSION_FILE}" ]] && [[ "$$(cat ${SCYLLA_VERSION_FILE})" =~ ^[0-9]+\.[0-9]+$$ ]]; then
		rm -f "${SCYLLA_VERSION_FILE}"
	fi

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Makefile Outdated
Comment thread Makefile
@nikagra
nikagra force-pushed the fix/DRIVER-924-tighten-version-check branch from d795cd2 to 5d5d6c4 Compare September 2, 2026 19:09
@nikagra
nikagra requested review from dkropachev and a balanced review from Copilot September 3, 2026 13:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The shared validation is consistently applied and addresses the documented cache and environment bypasses.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Comment thread Makefile Outdated
@nikagra
nikagra force-pushed the fix/DRIVER-924-tighten-version-check branch 2 times, most recently from bdda140 to 4cd82b9 Compare September 4, 2026 14:37
@nikagra
nikagra requested a balanced review from Copilot September 4, 2026 14:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The shared grammar is consistently applied and the identified cache and environment bypasses are addressed.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Comment thread Makefile Outdated
Comment thread Makefile Outdated
One grammar now decides what counts as a usable version, at every point that
accepts one. It could otherwise be bypassed: eviction rejected only a bare
MAJOR.MINOR, so a cached '4.1.' was served; the exemption flag was never
initialised; and the four targets handing a version to CCM never checked what
CI passes them. A warm cache also skipped the GITHUB_OUTPUT write.

A pre-release label must carry its own number: CCM strips a trailing -x86_64,
so 6.2.0-dev-aarch64 would reach it as the moving 6.2.0-dev. A bare label is
exact only before a dated build id such as 2024.2.3-0.20250108.931ce203dcf5.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018vGi2zZJxw51iagdPmVa4t
@nikagra
nikagra force-pushed the fix/DRIVER-924-tighten-version-check branch from 4cd82b9 to fe20a88 Compare September 8, 2026 17:21
@nikagra
nikagra requested a balanced review from Copilot September 8, 2026 17:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The shared validation closes the identified bypasses, and the complete CI matrix passes.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@dkropachev dkropachev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved. Validation is consistent and CI passes. Minor follow-up: _VERSION_TAIL still accepts trailing separators such as 6.2.3-rc0.foo.; please tighten before merge if practical.

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.

3 participants