storage: remove dependency to sql/catalog/bootstrap#83719
storage: remove dependency to sql/catalog/bootstrap#83719craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)
pkg/storage/BUILD.bazel line 192 at r1 (raw file):
disallowed_list = [ "//pkg/sql/catalog/bootstrap", ],
I don't think this does what you think it does. I think this only ensure that the library itself does not depend on bootstrap. Sadly, I don't think there's a way right now to represent the test dependencies.
Code quote:
disallowed_imports_test(
src = "storage",
disallowed_list = [
"//pkg/sql/catalog/bootstrap",
],pkg/storage/helpers_test.go line 11 at r1 (raw file):
// licenses/APL.txt. package storage_test
consider calling this file external_helpers_test.go. I think of helpers_test.go as the conventional file for forwarding unexported references from the current packages to tests defined in the _test package. This is going the other way.
pkg/storage/mvcc_test.go line 5018 at r1 (raw file):
} var TestingUserTableDataMin func() roachpb.Key
note that this is injected via external_helpers_test.go, same for the other var
0e2f329 to
bc7e733
Compare
Previously, tests in `pkg/storage` depended on `sql/catalog/bootstrap`. This was inadequate/weird because `storage` is a much lower layer in the architectural stack. It will also help prevent dependency cycles in other PRs when we introduce depencies (see cockroachdb#82172 if interested). Release note: None
bc7e733 to
6ba0ac6
Compare
Xiang-Gu
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/BUILD.bazel line 192 at r1 (raw file):
Previously, ajwerner wrote…
I don't think this does what you think it does. I think this only ensure that the library itself does not depend on
bootstrap. Sadly, I don't think there's a way right now to represent the test dependencies.
So, this rule will ignore files that end with *_test.go, so even if one *_test.go file (packaged storage) imports /pkg/sql/catalog/bootstrap, it won't catch and disallow it, right?
pkg/storage/mvcc_test.go line 5018 at r1 (raw file):
Previously, ajwerner wrote…
note that this is injected via
external_helpers_test.go, same for the other var
done
pkg/storage/helpers_test.go line 11 at r1 (raw file):
Previously, ajwerner wrote…
consider calling this file
external_helpers_test.go. I think ofhelpers_test.goas the conventional file for forwarding unexported references from the current packages to tests defined in the_testpackage. This is going the other way.
done
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status:
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Previously, tests in
pkg/storagedepended onsql/catalog/bootstrap.This was inadequate/weird because
storageis a much lower layer in thearchitectural stack. It will also help prevent dependency cycles in
other PRs when we introduce depencies (see #82172 if interested).
Release note: None