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

Adding clientv3 import alias to match usage in register_test.go. #12564

Merged
merged 3 commits into from
Jan 19, 2021
Merged

Adding clientv3 import alias to match usage in register_test.go. #12564

merged 3 commits into from
Jan 19, 2021

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 15, 2020

No description provided.

@@ -18,7 +18,7 @@ import (
"testing"
"time"

"go.etcd.io/etcd/client/v3"
clientv3 "go.etcd.io/etcd/client/v3"
Copy link
Contributor Author

@dhermes dhermes Dec 15, 2020

Choose a reason for hiding this comment

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

Note this came up because I am trying to create a fork (internal to my company) that can move past google.golang.org/grpc@v1.29.1. In order to do this, the usage of grpc/naming would need to be removed, though the suggested replacement in grpc/resolver isn't exactly sufficient for how it gets used here.

I came across this line because in my fork the c *clientv3.Client on line 71 caused my editor to automatically convert this import to go.etcd.io/etcd/clientv3 which caused a recursive reference outside of the repo (which in turn brought back the grpc/naming dependency).

In the process I noticed that the root go.mod points at tests but tests also thinks it needs to point at the root go.mod.

ISTM the go.etcd.io/etcd/v3 => ../ replace line in tests/go.mod should be removed and potentially a replace line like go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY should be added? (I can do that in this PR if you like)

@@ -30,7 +30,6 @@ replace (
go.etcd.io/etcd/pkg/v3 => ../pkg
go.etcd.io/etcd/raft/v3 => ../raft
go.etcd.io/etcd/server/v3 => ../server
// go.etcd.io/etcd/v3 => ../
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put this import in the replace block currently at the bottom of this file, if desired:

replace (
	go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
	go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also worth noting, this change doesn't fall under the purview of my PR subject, if you'd like I can create a second PR or make a more all-encompassing PR subject.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I would put this one /etcd/v3 to target to the ./FORBIDDEN_DEPENDENCY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 15, 2020

Not sure if this would be helpful but I created a quasi-tree of imports when I was trying to understand the issue (thought ordering of re-generating go.sum may be part of the culprit):

0: go.etcd.io/etcd/api/v3
0: go.etcd.io/etcd/pkg/v3
0: go.etcd.io/etcd/tools/v3
-----------------------------
1: go.etcd.io/etcd/client/v2 (requires 0:{api,pkg})
1: go.etcd.io/etcd/client/v3 (requires 0:{api,pkg})
1: go.etcd.io/etcd/raft/v3 (requires 0:{pkg})
-----------------------------
2: go.etcd.io/etcd/server/v3 (requires 0:{api,pkg}, 1:{client/v2,client/v3,raft})
-----------------------------
3: go.etcd.io/etcd/etcdctl/v3 (requires 0:{api,pkg}, 1:{client/v2,client/v3,raft}, 2:{server})
-----------------------------
4: go.etcd.io/etcd/tests/v3 (requires 0:{api,pkg}, 1:{client/v2,client/v3,raft}, 2:{server}, 3:{etcdctl})
-----------------------------
5: go.etcd.io/etcd/v3 (requires 0:{api,pkg}, 1:{client/v2,client/v3,raft}, 2:{server}, 4:{tests})

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

@@ -30,7 +30,6 @@ replace (
go.etcd.io/etcd/pkg/v3 => ../pkg
go.etcd.io/etcd/raft/v3 => ../raft
go.etcd.io/etcd/server/v3 => ../server
// go.etcd.io/etcd/v3 => ../
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I would put this one /etcd/v3 to target to the ./FORBIDDEN_DEPENDENCY

@ptabor ptabor merged commit 5dcd459 into etcd-io:master Jan 19, 2021
@dhermes dhermes deleted the patch-1 branch January 19, 2021 22:45
dhermes pushed a commit to blend/go.etcd.io-etcd that referenced this pull request Jan 20, 2021
See etcd-io#12564 (comment)
for how I determined which order to visit packages in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants