pkg/util/version: correctly handle CRDB versions

[dcrosta]

CRDB versions are not semantic versions [1], but the version library was designed to work with & correctly handle semvers. This change, which ports the version library from Cockroach Cloud into CRDB, updates CRDB's version library to:

• More-strictly validate actual, valid CockroachDB versions
• Correctly order actual CockroachDB versions [2]
• Add a MajorVersion type to capture & order CRDB major versions
• Remove the word "minor" from the Version type [1]

[1] The "minor" version -- the Y in vX.Y.Z -- does not carry the same meaning as in semver

[2] In particular, the "-cloudonly" release phase should sort after "-rc"; semver sorts these phases alphabetically, but we must not, in order to correctly order versions

This review is split into two commits

• The first which imports the cloud version.go and major_version.go, and adjusts its usage throughout the codebase, but (mostly) doesn't change version_test.go; this should (hopefully) make it easier to understand what behavior changes the revised Version type makes, although the old test suite was not very very comprehensive
• The second imports the cloud version_test.go which more completely tests all the behavior of the revised Version type

Issue: RE-814
Release note: None

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[github-actions]

⚪ Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note	Threshold
⚪ sec/op	9.536m ±6%	9.556m ±9%	~	p=0.912 n=10	3.0%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	10.16k ±0%	10.15k ±1%	~	p=0.912 n=10	2.0%
⚪ B/op	2.181Mi ±1%	2.179Mi ±0%	~	p=0.853 n=10	2.0%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/d3b559b/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d3b559b07564c66dac9a387a8abc92972a1f90c5/bin/pkg_sql_tests benchdiff/d3b559b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d3b559b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7b04347/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7b043472f92549bbefd6e718639e29d09afe2c5d/bin/pkg_sql_tests benchdiff/7b04347/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7b04347/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=7b04347 --new=d3b559b ./pkg/sql/tests

🟡 Sysbench [KV, 1node, local, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note	Threshold
🟡 sec/op	714.9µ ±1%	721.9µ ±1%	+0.99%	p=0.000 n=10	2.0%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
⚪ allocs/op	446.0 ±0%	446.0 ±0%	~	p=1.000 n=10	1.5%
⚪ B/op	254.7Ki ±0%	254.8Ki ±0%	~	p=0.436 n=10	1.5%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/d3b559b/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d3b559b07564c66dac9a387a8abc92972a1f90c5/bin/pkg_sql_tests benchdiff/d3b559b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d3b559b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7b04347/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7b043472f92549bbefd6e718639e29d09afe2c5d/bin/pkg_sql_tests benchdiff/7b04347/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7b04347/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_read_only$ --old=7b04347 --new=d3b559b ./pkg/sql/tests

🟡 Sysbench [KV, 1node, local, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note	Threshold
🟡 sec/op	1.431m ±1%	1.441m ±0%	+0.67%	p=0.005 n=10	2.5%
⚪ errs/op	0.000 ±0%	0.000 ±0%	~	p=1.000 n=10	0.0%
🟡 allocs/op	1.399k ±0%	1.400k ±0%	+0.04%	p=0.014 n=10	1.8%
⚪ B/op	290.5Ki ±1%	290.7Ki ±1%	~	p=0.160 n=10	1.8%

Reproduce

benchdiff binaries:

mkdir -p benchdiff/d3b559b/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d3b559b07564c66dac9a387a8abc92972a1f90c5/bin/pkg_sql_tests benchdiff/d3b559b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d3b559b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7b04347/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7b043472f92549bbefd6e718639e29d09afe2c5d/bin/pkg_sql_tests benchdiff/7b04347/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7b04347/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_write_only$ --old=7b04347 --new=d3b559b ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/d3b559b07564c66dac9a387a8abc92972a1f90c5/13463043401-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/7b043472f92549bbefd6e718639e29d09afe2c5d/13463043401-1/\* old/

Legend

• ⚪ Neutral: No significant performance change.
• 🟡 Warning: Slight degradation, likely due to variance, but still within thresholds.
• 🔴 Regression: Likely performance regression, requiring investigation.
• 🟢 Improvement: Possible performance gain.

No regressions detected!

built with commit: d3b559b07564c66dac9a387a8abc92972a1f90c5

[srosenberg]

Given the changes touch the mixedversion framework, I triggered a smoke test, which should execute nearly all mixedversion tests: GCE

[kenliu-crl]

I think @RaduBerinde and @dt have some historical knowledge around this, maybe worth reviewing

[RaduBerinde]

Very nice!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @golgeek, @jeffswenson, and @rafiss)

---

pkg/util/version/version.go line 31 at r3 (raw file):

)

// Version represents a CockroachDB version. Versions consist of two "main" parts:

This is a CockroachDB binary version (as opposed to cluster version), correct? (we should mention it)

---

pkg/util/version/version.go line 56 at r3 (raw file):

// Major returns the version's MajorVersion (the "vX.Y" part)
func (v Version) Major() MajorVersion {
	return MajorVersion{v.year, v.ordinal}

[nit] Use explicit field names

---

pkg/util/version/version.go line 244 at r3 (raw file):

			// handle -alpha.1, -rc.3, etc
			if phase := submatch(pat, matches, "phase"); phase != "" {
				v.phase = preReleasePhase[phase]

Should we error out when the phase is not in the map?

---

pkg/util/version/major_version.go line 17 at r3 (raw file):

)

type MajorVersion struct {

[nit] A comment here would be nice (mention it is specifically a Cockroach version).

[rafiss]

nice work! thanks for taking this on

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @dcrosta, @golgeek, and @jeffswenson)

---

pkg/util/version/major_version.go line 17 at r1 (raw file):

)

type MajorVersion struct {

nit: add a comment to this type

---

pkg/util/version/major_version.go line 24 at r3 (raw file):

	majorVersionRE := regexp.MustCompile(`^v(0|[1-9][0-9]*)\.([1-9][0-9]*)$`)
	if !majorVersionRE.MatchString(versionStr) {
		return MajorVersion{}, errors.Newf("not a valid CRDB major version: %s", versionStr)

super nit: let's use CockroachDB here as this could theoretically appear in a public facing message

---

pkg/util/version/major_version.go line 62 at r3 (raw file):

	return m.Compare(MajorVersion{}) == 0
}

nit: this type should implement redact.SafeFormatter, so that it does not get redacted when it is logged using a format string.

an example is roachpb.Version:

cockroach/pkg/roachpb/version.go

Line 64 in e083dbc

func (v Version) SafeFormat(p redact.SafePrinter, _ rune) {

ours will be even simpler:

// SafeFormat implements the redact.SafeFormatter interface.
func (m MajorVersion) SafeFormat(p redact.SafePrinter, _ rune) {
	p.Printf("v%d.%d", m.Year, v.Ordinal)
}

and let's also add this near the type declaration to enforce that the type implements redact.SafeFormatter:

var _ redact.SafeFormatter = MajorVersion{}

once we have that, then String() should be implemented as:

func (m MajorVersion) String() string {
	return redact.StringWithoutMarkers(m)
}

---

pkg/util/version/version.go line 186 at r3 (raw file):

	}

	if phase, ok := phaseName[v.phase]; ok {

i think we should make the unhappy case a bit more explicit. i suggest:

	if phase, ok := phaseName[v.phase]; ok {
		return fmt.Sprintf("v%d.%d.%d-%s.%d", v.year, v.ordinal, v.patch, phase, v.phaseOrdinal)
	} else {
		return fmt.Sprintf("v%d.%d.%d-INVALID.%d", v.year, v.ordinal, v.patch, phase, v.phaseOrdinal)
	}

and let's handle the cloudonly case specially. my concern here is that a future change could make it so the phase is accidentally never shown if the wrong code is forgotten to be updated.

---

pkg/util/version/version_test.go line 111 at r3 (raw file):

		cmp  int
	}{
		// Typical comparison scenarios of common types of versions

i think cloudonly versions should be tested in this section as well.

---

pkg/util/version/version_test.go line 126 at r3 (raw file):

		// A version with any unrecognized custom suffix is considered earlier than the same version
		// with no suffix, since these suffixes are most commonly used for odd pre-release cases
		{"v1.2.3", "v1.2.3-foo", -1},

the comment says "A version with any unrecognized custom suffix is considered earlier than the same version with no suffix." am i reading it backwards, or doesn't that mean v1.2.3-foo should compare less than v1.2.3?

---

pkg/util/version/version_test.go line 130 at r3 (raw file):

		{"v1.2.3", "v1.2.3-4-foo", -1},
		{"v1.2.3-4", "v1.2.3-4-foo", -1},
	}

could there be some tests with versions based off nightly builds, which have a commit tag

---

pkg/cmd/roachtest/tests/tpcc.go line 368 at r3 (raw file):

	// from other configs. We should take another pass to figure out more current numbers.

	// We append "-0" to the version so that we capture all prereleases of the

nit: this comment should be updated

but i also have a general thought: it sure would be nice not to have to remember that we need to append -alpha.0 every time we want to check for the earliest possible version of a major release. is it worth keeping the vX.Y.0-0 shorthand?

[dcrosta]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @golgeek, @jeffswenson, @RaduBerinde, and @rafiss)

---

pkg/cmd/roachtest/tests/tpcc.go line 368 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: this comment should be updated

but i also have a general thought: it sure would be nice not to have to remember that we need to append -alpha.0 every time we want to check for the earliest possible version of a major release. is it worth keeping the vX.Y.0-0 shorthand?

If that's a valid version number, then we should handle it. Is it? The existing library is based on extant and known-valid versions in use in Cloud, which may not capture all the use cases in the DB, where there's various in-development unreleased versions, etc.

But I agree, having to remember this is a bit smelly.

I've also considered adding a .Earliest() or something like that, which would encapsulate this within the library -- so like version.MustParse("v25.1.0").Earliest() would be equivalent to version.MustParse("v25.1.0-alpha.0"), etc...

Thoughts?

---

pkg/util/version/major_version.go line 17 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] A comment here would be nice (mention it is specifically a Cockroach version).

Ack

---

pkg/util/version/major_version.go line 24 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

super nit: let's use CockroachDB here as this could theoretically appear in a public facing message

Done.

---

pkg/util/version/major_version.go line 62 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: this type should implement redact.SafeFormatter, so that it does not get redacted when it is logged using a format string.

an example is roachpb.Version:

cockroach/pkg/roachpb/version.go

Line 64 in e083dbc

func (v Version) SafeFormat(p redact.SafePrinter, _ rune) {

ours will be even simpler:

// SafeFormat implements the redact.SafeFormatter interface.
func (m MajorVersion) SafeFormat(p redact.SafePrinter, _ rune) {
	p.Printf("v%d.%d", m.Year, v.Ordinal)
}

and let's also add this near the type declaration to enforce that the type implements redact.SafeFormatter:

var _ redact.SafeFormatter = MajorVersion{}

once we have that, then String() should be implemented as:

func (m MajorVersion) String() string {
	return redact.StringWithoutMarkers(m)
}

Do we need to do this for Version as well?

Is it okay to have SafeFormat call String instead of the other way around? This will ease sharing between cloud and the database, I suspect. See the latest revision here for my proposal implemented that way...

---

pkg/util/version/version.go line 31 at r3 (raw file):

Previously, RaduBerinde wrote…

This is a CockroachDB binary version (as opposed to cluster version), correct? (we should mention it)

What's the difference? Is a cluster version specifically just the major part (eg "v25.1") ?

---

pkg/util/version/version.go line 186 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we should make the unhappy case a bit more explicit. i suggest:

	if phase, ok := phaseName[v.phase]; ok {
		return fmt.Sprintf("v%d.%d.%d-%s.%d", v.year, v.ordinal, v.patch, phase, v.phaseOrdinal)
	} else {
		return fmt.Sprintf("v%d.%d.%d-INVALID.%d", v.year, v.ordinal, v.patch, phase, v.phaseOrdinal)
	}

and let's handle the cloudonly case specially. my concern here is that a future change could make it so the phase is accidentally never shown if the wrong code is forgotten to be updated.

The result here is shown to users, so I don't want to put "INVALID" in it.

I think the risk of having unknown/unmapped phases is small -- both unlikely, because of the checks in Parse, and the risk is low, since this value is used for display to humans in the cloud UI. We won't ever show the version numbering parts of it wrong, we might just omit the phase label.

---

pkg/util/version/version.go line 244 at r3 (raw file):

Previously, RaduBerinde wrote…

Should we error out when the phase is not in the map?

The series of regexes already only matches valid, known phases; anything else goes into the customLabel field, but I will add it for safety/future-proofing.

---

pkg/util/version/version_test.go line 111 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think cloudonly versions should be tested in this section as well.

In the second commit, we test cloudonly ordering in both TestVersionOrdering and TestVersionCompare (the successor of the old TestCompare). Do you think there's a missing test case? I know following this diff is probably a challenge 😬

---

pkg/util/version/version_test.go line 126 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the comment says "A version with any unrecognized custom suffix is considered earlier than the same version with no suffix." am i reading it backwards, or doesn't that mean v1.2.3-foo should compare less than v1.2.3?

You're reading it right, but I wrote it wrong; the tested behavior in the second commit is the correct, expected behavior -- unrecognized custom suffixes are sorted after the regular version.

IMO, the ordering tests in the second commit are a lot easier to read & reason about -- they just have input lists, and expected output lists after sorting.

---

pkg/util/version/version_test.go line 130 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

could there be some tests with versions based off nightly builds, which have a commit tag

The "sorts custom builds after corresponding normal version" case tests v21.1.0-1-g9cbe7c5281 -- is that what you're looking for?

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @dcrosta, @golgeek, @jeffswenson, and @rafiss)

---

pkg/util/version/version.go line 31 at r3 (raw file):

Previously, dcrosta (Dan Crosta) wrote…

What's the difference? Is a cluster version specifically just the major part (eg "v25.1") ?

Cluster versions don't have a patch number or any of the other stuff, but they have an internal version (which happens during upgrades), written out as v24.3-2

---

pkg/util/version/version.go line 244 at r3 (raw file):

Previously, dcrosta (Dan Crosta) wrote…

The series of regexes already only matches valid, known phases; anything else goes into the customLabel field, but I will add it for safety/future-proofing.

Ah, missed that, cool!

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @dcrosta, @golgeek, @jeffswenson, and @rafiss)

---

pkg/util/version/major_version.go line 17 at r5 (raw file):

)

// A MajorVersion represents a CockroachDB major version or relesae series, ie "v25.1".

release

[dcrosta]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @golgeek, @jeffswenson, @RaduBerinde, and @rafiss)

---

pkg/util/version/version.go line 31 at r3 (raw file):

Previously, RaduBerinde wrote…

Cluster versions don't have a patch number or any of the other stuff, but they have an internal version (which happens during upgrades), written out as v24.3-2

Ah, ok, cool. I'll update the comment to clarify this means binary version, thanks for the explainer

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @dcrosta, @golgeek, @jeffswenson, and @RaduBerinde)

---

pkg/util/version/format.go line 14 at r7 (raw file):
unfortunately this implementation will still cause the version to show up as redacted. as per the comment on (redact.SafeWriter).Print:

	// Print emits its arguments separated by spaces.
	// For each argument it dynamically checks for the SafeFormatter or
	// SafeValue interface and either use that, or mark the argument
	// payload as unsafe.
	Print(args ...interface{})

the string argument that is passed in is not known at runtime to contain no sensitive data, so the redaction facilities must redact it. you could work around it with something likep.Print(redact.SafeString(m.String())), but that's a code smell.

you can read through these concepts here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/1824817806/Log+and+error+redactability

This will ease sharing between cloud and the database, I suspect. See the latest revision here for my proposal implemented that way...

could you say more about why implementing this the suggested way would cause problems for using this in Cloud? either way, you still need to import the package and the same dependencies, no? generally, i wouldn't expect the importing code to care about how a function was implemented.

---

also, is there a reason that these functions are defined in a separate file, rather than alongside the other functions for these types? normally, in the rest of the CRDB codebase, these functions would be in the same place as the type definition.

---

pkg/util/version/major_version.go line 62 at r3 (raw file):

Do we need to do this for Version as well?

yes, that's a good idea

Is it okay to have SafeFormat call String instead of the other way around?

not quite. i'm moving this comment thread over to the implementation of SafeFormat

---

pkg/util/version/version.go line 186 at r3 (raw file):

The result here is shown to users, so I don't want to put "INVALID" in it.

showing "INVALID" in the string is certainly a better outcome than accidentally showing the wrong thing to the user. if you don't like "INVALID," then how about "unknown"?

as you say, it should never happen. we should either panic with an assertion error, or else indicate somehow that an unexpected value appeared in the version. i don't think silently ignoring an unexpected value here is a good option.

---

pkg/util/version/version_test.go line 111 at r3 (raw file):

Previously, dcrosta (Dan Crosta) wrote…

In the second commit, we test cloudonly ordering in both TestVersionOrdering and TestVersionCompare (the successor of the old TestCompare). Do you think there's a missing test case? I know following this diff is probably a challenge 😬

thanks! i had missed the second commit.

---

pkg/util/version/version_test.go line 130 at r3 (raw file):

Previously, dcrosta (Dan Crosta) wrote…

The "sorts custom builds after corresponding normal version" case tests v21.1.0-1-g9cbe7c5281 -- is that what you're looking for?

yes, thanks. i had missed the second commit.

[dcrosta]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @golgeek, @jeffswenson, and @rafiss)

---

pkg/util/version/format.go line 14 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

unfortunately this implementation will still cause the version to show up as redacted. as per the comment on (redact.SafeWriter).Print:

	// Print emits its arguments separated by spaces.
	// For each argument it dynamically checks for the SafeFormatter or
	// SafeValue interface and either use that, or mark the argument
	// payload as unsafe.
	Print(args ...interface{})

the string argument that is passed in is not known at runtime to contain no sensitive data, so the redaction facilities must redact it. you could work around it with something likep.Print(redact.SafeString(m.String())), but that's a code smell.

you can read through these concepts here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/1824817806/Log+and+error+redactability

This will ease sharing between cloud and the database, I suspect. See the latest revision here for my proposal implemented that way...

could you say more about why implementing this the suggested way would cause problems for using this in Cloud? either way, you still need to import the package and the same dependencies, no? generally, i wouldn't expect the importing code to care about how a function was implemented.

---

also, is there a reason that these functions are defined in a separate file, rather than alongside the other functions for these types? normally, in the rest of the CRDB codebase, these functions would be in the same place as the type definition.

My thought -- maybe not a good one -- was that we could (continue) code-sharing by file-copying. That led to me putting these in a separate file, which would not need to be copied, as well as the direction in which these things interact.

I'm not 100% sure this is the right way to code-share, it may make more sense to import cockroach as a module in cloud. We don't do this today, so it'd be a pretty big new vendor addition, but I presume it would work.

Can you say more about why redact.SafeString(m.String()) is a smell? That seems like very straightforward code to me?

---

pkg/util/version/version.go line 186 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

The result here is shown to users, so I don't want to put "INVALID" in it.

showing "INVALID" in the string is certainly a better outcome than accidentally showing the wrong thing to the user. if you don't like "INVALID," then how about "unknown"?

as you say, it should never happen. we should either panic with an assertion error, or else indicate somehow that an unexpected value appeared in the version. i don't think silently ignoring an unexpected value here is a good option.

I think it actually probably makes sense to move this to cloud-specific code. What we do or don't include in a "display name" is a surface-specific concept. In cloud, we don't show "cloudonly.X"; but I'm not sure that would necessarily hold -- either for this phase or any other/future ones -- in the DB (eg in DB console, logs, etc). I'll remove DisplayName from this PR.

---

pkg/util/version/version_test.go line 111 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

thanks! i had missed the second commit.

Done.

---

pkg/util/version/version_test.go line 130 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

yes, thanks. i had missed the second commit.

Done.

[rickystewart]

I'm not 100% sure this is the right way to code-share, it may make more sense to import cockroach as a module in cloud. We don't do this today, so it'd be a pretty big new vendor addition, but I presume it would work.

Please do not do this. This creates LOTS of problems. Instead, put the code to be shared into its own repo.

[dcrosta]

I'm not 100% sure this is the right way to code-share, it may make more sense to import cockroach as a module in cloud. We don't do this today, so it'd be a pretty big new vendor addition, but I presume it would work.

Please do not do this. This creates LOTS of problems. Instead, put the code to be shared into its own repo.

Can you say more? Today, we do this import in some cases, without major issues I'm aware of...

[rickystewart]

Can you say more? Today, we do this import in some cases, without major issues I'm aware of...

Discussed offline, DM me or Dan if you are a CRL employee who would like more context on this.

[dcrosta]

Closing this in favor of #142900