-
Notifications
You must be signed in to change notification settings - Fork 33
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
Support peer provider plugins #156
Conversation
@kriskowal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @prashantv to be a potential reviewer. |
a3dd9d3
to
ab4abe6
Compare
Fixes #155 |
peerprovider/interface.go
Outdated
RegisterPeerProvider("file", filePeerProvider{}) | ||
} | ||
|
||
// Schemes returns supported peer provider protocol schemes. |
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.
Can we add unit tests in this package for:
- registering a new scheme
- making sure it's returned in
Schemes
- ensuring it's used when
Resolve
is called with that scheme
transport.go
Outdated
// Paths are a strict subset of the URL file protocol scheme. We | ||
// explicate the file protocol as a key into the peer provider | ||
// registry. | ||
if u.Scheme == "" { |
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.
Could we register the file provider under the ""
scheme and have this work without this if
?
@@ -241,6 +242,13 @@ func parseDefaultConfigs(parser *flags.Parser) error { | |||
} | |||
|
|||
func runWithOptions(opts Options, out output) { | |||
if opts.TOpts.HostPortFile == "?" { |
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.
we can unit test this in, main_test.go
has some integration tests that should have examples of how to do this.
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.
addressed
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
func TestResolveError(t *testing.T) { | ||
oldRegistry := registry | ||
defer func() { registry = oldRegistry }() | ||
registry = make(map[string]PeerProvider) |
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.
optionally: make a helper for this,
func stubRegister() func() {
oldRegistry := registry
registry = make(map[string]PeerProvider)
return func() { registry = oldRegistry }()
}
func TestResolveError(t *testing.T) {
defer stubRegistry()()
[...]
}
This change factors out a pluggable peer provider registry and extends the meaning of the
-P
flag to be a URI, currently only supporting the implicit or explicitfile:/
protocol scheme.