Introduce backoff to retry requests #10
Conversation
squat
left a comment
squat
left a comment
There was a problem hiding this comment.
by convention, all commit message summaries at CoreOS are <dirs-affected>: summary of commit, so commits that vendor code are normally vendor: .... the commit message for adding the dep should be:
vendor: add backoff lib
|
@squat Okay, good to know. The |
| } | ||
| } | ||
|
|
||
| // makeGHRequest takes an API function from the GitHub library |
There was a problem hiding this comment.
take these functions out of root.go
cmd/root.go
Outdated
| // nil HTTP response and a timeout error are returned. | ||
| // | ||
| // It is nearly identical to makeGHRequest, but returns a JIRA API response. | ||
| func makeJIRARequest(f func() (interface{}, *jira.Response, error)) (ret interface{}, res *jira.Response, err error) { |
There was a problem hiding this comment.
by convention, we do not normally use named returns. I would prefer to see:
var ret interface{}
var res *jira.Response
op := ...
...
return ret, res, nil
cmd/root.go
Outdated
| return nil, jira.CheckResponse(resp.Response) | ||
| } | ||
| project = *proj | ||
| project = *(proj.(*jira.Project)) |
There was a problem hiding this comment.
this is now a pointer to a pointer. is that what you want? also, make sure to check if the type assertion succeeds.
There was a problem hiding this comment.
Wouldn't a pointer to a pointer be project = &(proj.(*jira.Project))? That should dereference the pointer, so that project is just a jira.Project, no pointer.
cmd/root.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| ghIssues := i.([]*github.Issue) |
There was a problem hiding this comment.
check the type assertion to avoid panics later
cmd/root.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| jiraIssues := ji.([]jira.Issue) |
cmd/root.go
Outdated
| if err != nil { | ||
| log.Errorf("Error retrieving JIRA issue %s to get comments.", jIssue.Key) | ||
| } | ||
| issue := i.(*jira.Issue) |
cmd/root.go
Outdated
| log.Debugf("Error body: %s", body) | ||
| return err | ||
| } | ||
| jIssue = i.(*jira.Issue) |
cmd/root.go
Outdated
| log.Errorf("Error retrieving GitHub comments for issue #%d. Error: %s.", *ghIssue.Number, err) | ||
| return err | ||
| } | ||
| comments := c.([]*github.IssueComment) |
cmd/root.go
Outdated
| if err != nil { | ||
| log.Errorf("Error retrieving GitHub user %s. Error: %s", *ghComment.User.Login, err) | ||
| } | ||
| user := u.(*github.User) |
cmd/root.go
Outdated
| if err != nil { | ||
| log.Errorf("Error retrieving GitHub user %s. Error: %s", *ghComment.User.Login, err) | ||
| } | ||
| user := u.(*github.User) |
squat
left a comment
squat
left a comment
There was a problem hiding this comment.
Overall look ok, @LyonesGamer. There are a few comments that will be quick to fix. We really need to get the structural changes in before any more features are added because this is completely unmaintanable. Also, in the future, please squash the non-vendor commits. Reviewing this code is almost impossible because each individual commit does not show the full picture, but if you look at all the changes together then all the meaningful changes are lost in thousands of lines of vendor changes.
cmd/root.go
Outdated
| log.Errorf("Get JIRA project did not return project! Value: %v", proj) | ||
| return nil, errors.New(fmt.Sprintf("Get project failed: expected *jira.Project; got %T", proj)) | ||
| } | ||
| project = *(proj.(*jira.Project)) |
There was a problem hiding this comment.
this should be done in a single type assertion above. you do
project, ok := proj.(...)
if !ok {
return ...
}
... continue using project.
cmd/root.go
Outdated
| return jiraClient.Issue.Search(jql, nil) | ||
| }) | ||
| if err != nil { | ||
| log.Errorf("Error retrieving JIRA issues: %s", err) |
cmd/root.go
Outdated
| if err != nil { | ||
| log.Errorf("Error retrieving JIRA issues: %s", err) | ||
| defer res.Body.Close() | ||
| body, _ := ioutil.ReadAll(res.Body) |
There was a problem hiding this comment.
do not suppress this error. In this particular case, if there was an error making the request, then there is a chance that res.Body is nil.
| u, _, err := makeGHRequest(func() (interface{}, *github.Response, error) { | ||
| return ghClient.Users.Get(context.Background(), *ghComment.User.Login) | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
This is really weird, there should not be a period between these two clauses but rather a colon. Conventionally we do:
log.Errorf("error retrieving GitHub user %s: %v", user.Login, err)
cmd/root.go
Outdated
| user, ok := u.(*github.User) | ||
| if !ok { | ||
| log.Errorf("Get GitHub user did not return user! Got: %v", u) | ||
| return errors.New(fmt.Sprintf("Get GitHub user failed: expected *github.User; got %T", u)) |
There was a problem hiding this comment.
Errors should not start with a capital because it is convention to wrap errors in more errors and this results in capitalized words in the middle of sentences. if you use golint, these mistakes will be caught.
cmd/root.go
Outdated
| if !dryRun { | ||
| req, err := jClient.NewRequest("PUT", fmt.Sprintf("rest/api/2/issue/%s/comment/%s", jIssue.Key, jComment.ID), request) | ||
| if err != nil { | ||
| log.Errorf("Error creating comment update request: %s", err) |
Add the dependency on the github.com/cenkalti/backoff library. Apparently this also triggered the entire vendor directory to be refilled or something.
|
@squat Fixed all of your comments. |
2 participants
Depends on #9.
In the event that a request to JIRA or GitHub fails, retry the requests with an exponential backoff, until the request succeeds or sufficient time has passed.