Skip to content
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

go.mod: import etcd/raft at the new path #93190

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

pav-kv
Copy link
Collaborator

@pav-kv pav-kv commented Dec 7, 2022

The etcd/raft package has moved to a separate repository, and was renamed to go.etcd.io/raft. This commit imports the new path.

Fixes #93174
Epic: none

@pav-kv pav-kv requested review from a team as code owners December 7, 2022 10:34
@pav-kv pav-kv requested a review from a team December 7, 2022 10:34
@pav-kv pav-kv requested review from a team as code owners December 7, 2022 10:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pav-kv
Copy link
Collaborator Author

pav-kv commented Dec 7, 2022

I'm not sure yet how compatibility will work between nodes with the new and old protos. Technically it's just a rename, and the encoding shouldn't change, but it would be nice to confirm that the full message path/name is not used.

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Dec 7, 2022

I think the name only matters if we're using them in Any fields.

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks!

I checked upstream what we're bumping to here and it looks like it's basically "latest of our changes" and then there are some fixups that don't affect the raft code.

image

@tbg
Copy link
Member

tbg commented Dec 7, 2022

fmtsave has some issue it seems. I think you need to update src/.... tree to have a matching fake module as well.

Failed
=== RUN   Test
src/a/a.go:18:2: cannot find package "go.etcd.io/raft/v3" in any of:
	/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/10248/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test_/fmtsafe_test.runfiles/go_sdk/src/go.etcd.io/raft/v3 (from $GOROOT)
	/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/10248/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test_/fmtsafe_test.runfiles/com_github_cockroachdb_cockroach/pkg/testutils/lint/passes/fmtsafe/testdata/src/go.etcd.io/raft/v3 (from $GOPATH)
/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/10248/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test_/fmtsafe_test.runfiles/com_github_cockroachdb_cockroach/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go:18:2: could not import go.etcd.io/raft/v3 (invalid package name: "")
/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/10248/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test_/fmtsafe_test.runfiles/com_github_cockroachdb_cockroach/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go:55:8: undeclared name: raft
/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/10248/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test_/fmtsafe_test.runfiles/com_github_cockroachdb_cockroach/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go:18:2: could not import go.etcd.io/raft/v3 (invalid package name: "")
/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/10248/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/testutils/lint/passes/fmtsafe/fmtsafe_test_/fmtsafe_test.runfiles/com_github_cockroachdb_cockroach/pkg/testutils/lint/passes/fmtsafe/testdata/src/a/a.go:55:8: undeclared name: raft
    analysistest.go:298: error analyzing fmtsafe@a: analysis skipped due to errors in package
    analysistest.go:298: error analyzing fmtsafe@a: analysis skipped due to errors in package
    analysistest.go:298: error analyzing fmtsafe@a.test: analysis skipped due to errors in package
--- FAIL: Test (3.16s)

@tbg
Copy link
Member

tbg commented Dec 8, 2022

Still LGTM but looks like you need to re-do vendor because someone beat you to it. Pro tip, if you do it be ready to bors it soon thereafter or you get starved :-)

@pav-kv
Copy link
Collaborator Author

pav-kv commented Dec 8, 2022

There are some obscure lint failures in one of the targets that I'm still decoding.

@pav-kv pav-kv force-pushed the import_new_raft_package branch 2 times, most recently from a21bc87 to 06da857 Compare December 9, 2022 15:46
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 97 of 121 files at r1, 25 of 25 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)

@pav-kv pav-kv force-pushed the import_new_raft_package branch 2 times, most recently from 2f5ea24 to ddbe007 Compare December 19, 2022 10:37
@pav-kv pav-kv requested a review from a team as a code owner December 19, 2022 10:37
@pav-kv pav-kv requested a review from a team December 19, 2022 10:37
@pav-kv
Copy link
Collaborator Author

pav-kv commented Dec 19, 2022

make lint still fails with:

pkg/server/serverpb/status.proto:39:8:etcd/raft/v3/raftpb/raft.proto: does not exist
make: *** [Makefile:1373: bin/.gw_protobuf_sources] Error 100
make: *** Waiting for unfinished jobs....
pkg/kv/kvserver/kvserverpb/raft.proto:21:8:etcd/raft/v3/raftpb/raft.proto: does not exist
make: *** [Makefile:1365: bin/.go_protobuf_sources] Error 100

... even though the ./dev / bazel builds are happy.

It also says that the makefile is deprecated, so I don't know how much effort should be put into fixing this. @tbg any ideas?

@pav-kv
Copy link
Collaborator Author

pav-kv commented Dec 19, 2022

Now for some reason make generates this import path:

raftpb "github.com/cockroachdb/cockroach/pkg/raft/v3/raftpb"

instead of etcd raft

@pav-kv pav-kv force-pushed the import_new_raft_package branch 2 times, most recently from db9e1a0 to 3dec0cb Compare December 19, 2022 18:40
The etcd/raft package has moved to a separate repository, and was renamed to
go.etcd.io/raft. This commit imports the new path.

Epic: none

tmp
@pav-kv
Copy link
Collaborator Author

pav-kv commented Dec 19, 2022

bors r=tbg,nvanbenschoten

@craig
Copy link
Contributor

craig bot commented Dec 19, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Dec 19, 2022

Build succeeded:

@craig craig bot merged commit 2457613 into cockroachdb:master Dec 19, 2022
@pav-kv pav-kv deleted the import_new_raft_package branch December 19, 2022 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go.mod: use new etcd/raft import path
5 participants