Uh oh!
There was an error while loading. Please reload this page.
Create comments in JIRA after create and update - #6
Conversation
squat
left a comment
There was a problem hiding this comment.
This looks good overall, however, the updateComment, createComment etc functions must be broken out into a new file, preferably in a new package. this file in unwieldy and contains code that is not relevant to cmd handling
| } | ||
| ctx := context.Background() | ||
| repo := strings.Split(rootCmdCfg.GetString("repo-name"), "/") |
There was a problem hiding this comment.
you do this same parsing in multiple places. repo should be a field on a config object so it can be reused.
| return err | ||
| } | ||
| res, err := jClient.Do(req, nil) // Response should be 204 No Content |
There was a problem hiding this comment.
if it should be 204, why not confirm it by comparing to http.StatusNoContent?
| res, err := jClient.Do(req, nil) // Response should be 204 No Content | ||
| if err != nil { | ||
| log.Errorf("Error updating comment: %s", err) | ||
| body, _ := ioutil.ReadAll(res.Body) |
There was a problem hiding this comment.
do not ignore the error returned by ReadAll; handle it as necessary
| return err | ||
| } else if err = jira.CheckResponse(res.Response); err != nil { | ||
| log.Errorf("Error updating comment: %s", err) | ||
| body, _ := ioutil.ReadAll(res.Body) |
| jComment, res, err := jClient.Issue.AddComment(jIssue.ID, jComment) | ||
| if err != nil { | ||
| log.Errorf("Error creating JIRA comment on issue %s. Error: %s", jIssue.Key, err) | ||
| body, _ := ioutil.ReadAll(res.Body) |
| if err := updateIssue(*ghIssue, jIssue, jiraClient); err != nil { | ||
| log.Errorf("Error updating issue %s. Error: %v", jIssue.Key, err) | ||
| if err := updateIssue(*ghIssue, jIssue, ghClient, jiraClient); err != nil { | ||
| log.Errorf("Error updating issue %s. Error: %s", jIssue.Key, err) |
There was a problem hiding this comment.
could you format the error with %v
| Direction: "asc", | ||
| }) | ||
| if err != nil { | ||
| log.Errorf("Error retrieving GitHub comments for issue #%d. Error: %s.", *ghIssue.Number, err) |
| user, _, err := ghClient.Users.Get(context.Background(), *ghComment.User.Login) | ||
| if err != nil { | ||
| log.Errorf("Error retrieving GitHub user %s. Error: %s", *ghComment.User.Login, err) |
| log.Errorf("Error retrieving GitHub user %s. Error: %s", *ghComment.User.Login, err) | ||
| } | ||
| body := fmt.Sprintf("Comment (ID %d) from GitHub user %s", *ghComment.ID, user.GetLogin()) | ||
| if user.GetName() != "" { |
There was a problem hiding this comment.
To simplify this a little bit, how about making the only bit that is conditioned on GetName() != "" be the part that actually appends the username:
ifuser.GetName() !="" {
body-fmt.Sprintf("%s (%s)", body, user.GetName())
}
body=fmt.Sprintf(
"%s at %s:\n\n%s",
body,
ghComment.CreatedAt.Format(commentDateFormat),
*ghComment.Body,
)This cleans up the logic, de-dupes the code and shortens everything a little.
| Body: body, | ||
| } | ||
| req, err := jClient.NewRequest("PUT", fmt.Sprintf("rest/api/2/issue/%s/comment/%s", jIssue.Key, jComment.ID), request) |
There was a problem hiding this comment.
please add a TODO
TODO(LyonesGamer): abstract this into its own method
| jComment, res, err := jClient.Issue.AddComment(jIssue.ID, jComment) | ||
| if err != nil { | ||
| log.Errorf("Error creating JIRA comment on issue %s. Error: %s", jIssue.Key, err) |
| } | ||
| found = true | ||
| updateComment(*ghComment, jComment, jIssue, ghClient, jClient) |
There was a problem hiding this comment.
if you have 20 Jira comments and you find the GitHub comment on the first 1, you will loop over the remaining 19 comments. Instead, you should break after updateComment.
| var comments []jira.Comment | ||
| if issue.Fields.Comments == nil { | ||
| log.Debugf("JIRA issue %s has no comments.", jIssue.Key) | ||
| comments = make([]jira.Comment, 0) |
There was a problem hiding this comment.
you do not need to call make on the slice; in fact, you are allocating an array that will never be used. if you need to append elements to the slice, the built-in append function will lazily allocate and re-allocate a backing array for the slice as needed.
| "time" | ||
| "regexp" | ||
There was a problem hiding this comment.
there should not be any newlines between these stdlibs
| "strconv" | ||
| "net/http" | ||
There was a problem hiding this comment.
this newline separating stdlib and remote imports is good
| var comments []jira.Comment | ||
| if issue.Fields.Comments == nil { | ||
| log.Debugf("JIRA issue %s has no comments.", jIssue.Key) | ||
| comments = nil |
There was a problem hiding this comment.
you should delete this line entirely; it is not needed
There was a problem hiding this comment.
Oops. Yep, that's a holdover from C programming, and the assumption that var comments []jira.Comment will leave comments uninitialized and unusable.
Thanks for making those changes. Thoughts on: #6 (comment)? |
squat
commented
Jul 20, 2017
@LyonesGamer one last thing: #6 (comment) |
MorganEPatch
commented
Jul 20, 2017
Oops. Can't believe I missed that one. |
MorganEPatch
commented
Jul 20, 2017
@squat 👍 |
squat
left a comment
There was a problem hiding this comment.
Just gave it one last look and I found two last things that slipped through. Once these are done, lets squash and merge.
| if err != nil { | ||
| log.Errorf("Error updating JIRA issue %s: %v", jIssue.Key, err) | ||
| defer res.Body.Close() | ||
| body, _ := ioutil.ReadAll(res.Body) |
There was a problem hiding this comment.
Just realized this one slipped through the cracks
There was a problem hiding this comment.
I hate it when I accidentally revert a fix I've already made... 😞
| log.Errorf("Error creating JIRA issue: %s", err) | ||
| log.Errorf("Error creating JIRA issue: %v", err) | ||
| defer res.Body.Close() | ||
| body, _ := ioutil.ReadAll(res.Body) |
This PR adds the createComments, createComment, and updateComment functions, which add any new comments from a GitHub issue to a JIRA issue, and updates existing ones if they've been edited. The comments are created by the user running the tool; a header will be created to track metadata.
MorganEPatch
commented
Jul 20, 2017
@squat Fixed, squashed, and ready to merge! |
squat
commented
Jul 20, 2017
|
This PR builds on #4, and is thus blocked on it.
This PR adds the createComments() function, which adds any new comments on a given GitHub issue to the to given JIRA issue, and updates existing issues if they've been edited on GitHub. Currently, the comment is made by the user under which the tool is running.