Retry SSH resolution 5 times #2934
Conversation
There was a problem hiding this comment.
lfsapi/client.go
Outdated
| httpRE = regexp.MustCompile(`\Ahttps?://`) | ||
| UserAgent = "git-lfs" | ||
| httpRE = regexp.MustCompile(`\Ahttps?://`) | ||
| maxSshAuthenticateRetries = 5 |
There was a problem hiding this comment.
Do you think that this could be an instance variable belonging to *lfsapi.Client? I don't think that this has to be in the package-global namespace, so I'd advocate for the former.
lfsapi/client.go
Outdated
| if err == nil { | ||
| return sshRes, nil | ||
| } | ||
|
|
||
| tracerx.Printf("ssh: %s failed, error: %s, message: %s", |
There was a problem hiding this comment.
Should these messages be changed to indicate when we are and aren't retrying? You can take a look at something similar, like this:
Line 493 in 8c8a711
lfsapi/client.go
Outdated
| func (c *Client) NewRequest(method string, e Endpoint, suffix string, body interface{}) (*http.Request, error) { | ||
| sshRes, err := c.SSH.Resolve(e, method) | ||
| if err != nil { | ||
| func (c *Client) sshResolveWithRetries(e Endpoint, method string) (sshAuthResponse, error) { |
There was a problem hiding this comment.
What do you think about changing the return type of this method to be:
(*sshAuthResponse, error)instead of
(sshAuthResponse, error)that way we can return nil to indicate a failure or missing sshAuthResponse, instead of sshAuthResponse{}, which could be misleading or confusing to callers of this method.
lfsapi/client.go
Outdated
| for i := 0; i < requests; i++ { | ||
| sshRes, err = c.SSH.Resolve(e, method) | ||
| if err == nil { | ||
| return sshRes, nil |
There was a problem hiding this comment.
Per my suggestion above, this line would change to:
return &sshRes, nil
lfsapi/client.go
Outdated
| return nil, errors.Wrap(err, sshRes.Message) | ||
| } | ||
| if len(sshRes.Message) > 0 { | ||
| return sshAuthResponse{}, errors.Wrap(err, sshRes.Message) |
There was a problem hiding this comment.
And this, (as well as the line below it, L51) would change to:
return nil, err
ttaylorr
changed the title
|
Hi @stanhu, I hope that it's OK I updated your branch myself to include feedback that I gave above and changes from master. |
|
@ttaylorr Thanks! |
Closes #2914