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
logic: keep cockroach-go logs in case of failure #104876
logic: keep cockroach-go logs in case of failure #104876
Conversation
bb57c78
to
406ac73
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.
lgtm, but there's a small nit which you can address and feel free to merge after
pkg/sql/logictest/logic.go
Outdated
@@ -1287,6 +1300,7 @@ func (t *logicTest) newTestServerCluster(bootstrapBinaryPath string, upgradeBina | |||
t.clusterCleanupFuncs = append(t.clusterCleanupFuncs, ts.Stop) |
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.
nit: as long as the cleanupFuncs are being added here, should we just have your new callback get appended here too, rather than returning it. either way, it seems better to be consistent either append both cleanupFunctions here, or to the ts.Stop
call in the returned function.
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.
Right, I don't know what I was thinking. Fixed.
This commit makes use of the new `CockroachLogsDirOpt` option introduced in [1] to pass a custom directory where cockroach logs will be stored. This directory is relative to TMPDIR, which is part of the artifacts path when the test runs in CI. With this change, when a logic test using the cockroach-go config fails, we'll be able to see both the test logs as well as the cockroach logs themselves. The latter will be in the `cockroach.stderr` file. [1] cockroachdb/cockroach-go#170 Epic: none Release note: None
406ac73
to
8069f3a
Compare
TFTR! bors r=rafiss |
Build succeeded: |
This commit makes use of the new
CockroachLogsDirOpt
option introduced in [1] to pass a custom directory where cockroach logs will be stored. This directory is relative to TMPDIR, which is part of the artifacts path when the test runs in CI. With this change, when a logic test using the cockroach-go config fails, we'll be able to see both the test logs as well as the cockroach logs themselves. The latter will be in thecockroach.stderr
file.[1] cockroachdb/cockroach-go#170
Epic: none
Release note: None