sql: add crdb_internal.release_series builtin
#113804
Conversation
|
This change is |
RaduBerinde
force-pushed
the
release-series
branch
4 times, most recently
from
45571b4 to
4f31543
Compare
RaduBerinde
force-pushed
the
release-series
branch
from
4f31543 to
cbe481b
Compare
There was a problem hiding this comment.
LGTM -- thank you! looks great
Reviewed 12 of 14 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @RaduBerinde, and @rail)
pkg/sql/sem/builtins/builtins.go line 5013 at r1 (raw file):
} if version.Less(clusterversion.MinSupported.Version()) || clusterversion.Latest.Version().Less(version) { return nil, errors.Newf(
maybe add test case to verify that less than MinSupport or greater than Latest returns error as expected?
(e.g. maybe to builtins_test.go?)
pkg/upgrade/upgrades/builtins_test.go line 63 at r1 (raw file):
sqlDB.CheckQueryResults(t, "SELECT crdb_internal.release_series(version) FROM [SHOW CLUSTER SETTING version]", [][]string{{clusterversion.Latest.ReleaseSeries().String()}}) }
maybe add test case to verify that less than MinSupport or greater than Latest returns error as expected?
This commit adds a built-in that resolves dev versions (e.g. 23.2-10) to the corresponding release series (e.g. 24.1). This allows us to clean up some tests and make them stable - they will no longer need to be adjusted when minting a release. Informs: cockroachdb#112629 Release note: None
RaduBerinde
force-pushed
the
release-series
branch
from
cbe481b to
15f92e5
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/sem/builtins/builtins.go line 5013 at r1 (raw file):
Previously, celiala wrote…
maybe add test case to verify that less than MinSupport or greater than Latest returns error as expected?
(e.g. maybe to builtins_test.go?)
Added to crdb_internal
|
bors r+ |
There was a problem hiding this comment.
Reviewed 1 of 14 files at r1.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/clusterversion/dev_offset.go line 90 at r2 (raw file):
// removeDevOffset removes DevOffset from the given version, if it was applied. func removeDevOffset(v roachpb.Version) roachpb.Version {
nit: maybe rename this to maybeRemoveDevOffset for consistency
|
Build succeeded: |
|
Previously, andyyang890 (Andy Yang) wrote…
|
There was a problem hiding this comment.
Reviewable status:
pkg/clusterversion/dev_offset.go line 90 at r2 (raw file):
Previously, RaduBerinde wrote…
maybeRemovesuggests that it might not be removed in some cases..
Hmm yeah I guess it's ambiguous, I personally interpret maybeRemove as "remove if it was applied, otherwise do nothing"
|
Previously, andyyang890 (Andy Yang) wrote…
Maybe |
There was a problem hiding this comment.
Reviewable status:
pkg/clusterversion/dev_offset.go line 90 at r2 (raw file):
Previously, RaduBerinde wrote…
Maybe
withoutDevOffset?
Works for me! Also ok with just keeping it as-is, up to you.
This commit adds a built-in that resolves dev versions (e.g. 23.2-10)
to the corresponding release series (e.g. 24.1). This allows us to
clean up some tests and make them stable - they will no longer need to
be adjusted when minting a release.
Informs: #112629
Release note: None