-
Notifications
You must be signed in to change notification settings - Fork 38
version: update version check to use safe introspection tool #154
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
version: update version check to use safe introspection tool #154
Conversation
7aac338 to
e8d6fec
Compare
e8d6fec to
86f1a80
Compare
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.
Pull Request Overview
This PR fixes an issue with ORM examples by modifying how version strings are extracted from CockroachDB. The SHOW crdb_version query returns a fuller string (e.g., "CockroachDB v21.1.0") rather than just the version, requiring the Parse function to extract the version substring instead of requiring an exact match.
Key changes:
- Modified
version.Parse()to extract version substrings from larger strings usingFindStringinstead of requiring exact matches - Updated SQL query in tests from
crdb_internal.node_build_infotoSHOW crdb_version - Reformatted documentation comments to follow Go conventions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| version/version.go | Modified Parse function to extract version substrings, removed regex anchors, and improved doc formatting |
| testing/main_test.go | Changed database version query to use SHOW crdb_version instead of internal table lookup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
version/version.go
Outdated
| // version (as per https://semver.org/spec/v2.0.0.html) in the format: | ||
| // "vMINOR.MAJOR.PATCH[-PRERELEASE][+METADATA]". | ||
| // | ||
| // "vMINOR.MAJOR.PATCH[-PRERELEASE][+METADATA]". |
Copilot
AI
Nov 17, 2025
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.
The version format in the documentation is incorrect. It should be "vMAJOR.MINOR.PATCH" not "vMINOR.MAJOR.PATCH". The order of MAJOR and MINOR is swapped.
| // "vMINOR.MAJOR.PATCH[-PRERELEASE][+METADATA]". | |
| // "vMAJOR.MINOR.PATCH[-PRERELEASE][+METADATA]". |
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.
This seems like a good suggestion. This pre-existed your change, but since you are changing things close by, it makes sense to fix with your change.
a5c46ae to
fa9f616
Compare
a7ab5a5 to
b5ed13e
Compare
spilchen
left a comment
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.
Looks good. We should probably resolve the linter error, although maybe in a separate PR as it seems unrelated to your change and a problem with the Go toolchain used in the CI. There aren't many changes to this repo (the last CI run was from Feb 2024), which is why it's only be noticed now.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion
version/version.go
Outdated
| // version (as per https://semver.org/spec/v2.0.0.html) in the format: | ||
| // "vMINOR.MAJOR.PATCH[-PRERELEASE][+METADATA]". | ||
| // | ||
| // "vMINOR.MAJOR.PATCH[-PRERELEASE][+METADATA]". |
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.
This seems like a good suggestion. This pre-existed your change, but since you are changing things close by, it makes sense to fix with your change.
b5ed13e to
f434efa
Compare
|
Got it, I'll make that change, I'll also see if I can figure out how to fix the linting issue, but time box it. |
Previously, we would query `crdb_internal.node_build_info` table to get a cluster's version info. Starting at 26.1, we'll be blocking access to crdb_internal tables, as they're considered part of the "unsafe internals". To mitigate this issue before release, we'll use instead `SHOW crdb_version` to collect version information about the cluster. Fixes: #154660 Release note: none
f434efa to
600b4c0
Compare
Previously, we would query
crdb_internal.node_build_infotable to geta cluster's version info. Starting at 26.1, we'll be blocking access to
crdb_internal tables, as they're considered part of the "unsafe
internals". To mitigate this issue before release, we'll use instead
SHOW crdb_versionto collect version information about the cluster.Fixes: #154660
Release note: none