transport/ssh: allow passing SSH options #423
Conversation
plumbing/transport/ssh/common.go
Outdated
| var DefaultClient = NewClient() | ||
|
|
||
| // NewClient creates a new SSH client with the given options. | ||
| func NewClient(opts ...ClientOption) transport.Transport { |
There was a problem hiding this comment.
why not just accepting a ssh.ClientConfig?
There was a problem hiding this comment.
To play nice with other options that modify ssh.ClientConfig:
config := c.auth.clientConfig()
config.HostKeyCallback, err = c.auth.hostKeyCallback()
if err != nil {
return err
}
ssh.ClientConfig is created and initialized by client on each session. Some fields are set based on AuthMethod, which is not global, but per-session setting. Then setting the new options for ssh.ClientConfig are set globally for the transport, but applied per session after it has been created.
There was a problem hiding this comment.
You can archive the same just make a copy of the ssh.ClientConfig. And the API for the go-git user will be more simple and easy to use.
There was a problem hiding this comment.
That's right. The only difference is that global options could not override session values (e.g. adding a global override on every HostKeyCallback, such as an interceptor). Is that ok?
There was a problem hiding this comment.
you can, checking if is zero value before assign the HostKeyCallback
There was a problem hiding this comment.
But you cannot use it to have a global wrapper (instead of a global override). As in:
if c.HostKeyCallback != nil {
c.HostKeyCallback = globalWrapper(c.HostKeyCallback)
}
Note that we discussed about this offline and agreed to make the API simpler with just a default ssh.ClientConfig that is copied. I will update the PR accordingly.
Adds the possibility of passing options to SSH transport. Options have the form of functions modifying ssh.ClientConfig.
A global *ssh.ClientConfig override can be set. It will be use to override values of each SSH session.
Codecov Report
@@ Coverage Diff @@
## master #423 +/- ##
==========================================
- Coverage 78.05% 77.19% -0.87%
==========================================
Files 127 124 -3
Lines 9188 9023 -165
==========================================
- Hits 7172 6965 -207
- Misses 1236 1300 +64
+ Partials 780 758 -22
Continue to review full report at Codecov.
|
|
@mcuadros Is this what you had in mind? Note that it's still not tested. |
| return | ||
| } | ||
|
|
||
| vo := reflect.ValueOf(*overrides) |
There was a problem hiding this comment.
For do the copy should be enough something like:
var config *ssh.ClientConfig
*config = *overrides
There was a problem hiding this comment.
But we don't want to override values set for the session, such as HostKeyCallback.
Adds the possibility of passing options to SSH transport.
Options have the form of functions modifying ssh.ClientConfig.