Uh oh!
There was an error while loading. Please reload this page.
Retrieve issues from both services - #1
Conversation
| return nil | ||
| } | ||
| type Field struct { |
There was a problem hiding this comment.
Would you rename this to something more descriptive?
There was a problem hiding this comment.
Agreed. Maybe JIRAField?
| return err | ||
| } | ||
| for _, field := range *fields { |
There was a problem hiding this comment.
What will happen if we don't set ghIDFieldID?
There was a problem hiding this comment.
I added checks that these fields exist on the project. Later I can make some of them optional and have the tool recognize that, but for now they're all required. I'll document it in the README when I write that up.
| ghIssues, _, err := ghClient.Issues.ListByRepo(ctx, repo[0], repo[1], &github.IssueListByRepoOptions{ | ||
| Since: since, | ||
| ListOptions: github.ListOptions{ | ||
| PerPage: 100, |
There was a problem hiding this comment.
Will have to add support for pagination later?
There was a problem hiding this comment.
Yes, for now it just operates on the most recent page of results.
| if int64(*ghIssue.ID) == id { | ||
| found = true | ||
| if err := updateIssue(*ghIssue, jIssue); err != nil { | ||
| return err |
There was a problem hiding this comment.
It would probably be better to log this err and keep trucking - (and the same below).
kans
commented
Jul 12, 2017
@LyonesGamer If you have several more PRs chained after this and don't want to go through rebasing them all multiple times, you could just address the comments in a future PR. |
MorganEPatch
commented
Jul 12, 2017
I think it'll be fine; they're all linear, and the comments are mostly small changes so far anyway. They should rebase pretty easily. If there's a comment asking for a big change, I'll probably make it a new PR. |
| func Execute() { | ||
| if err := RootCmd.Execute(); err != nil { | ||
| fmt.Println(err) | ||
| log.Debugln(err) |
There was a problem hiding this comment.
Rather than log.Debug followed by os.Exit consider using just log.Fatal. It is more semantic and will appropriately exit the process.
| } | ||
| } | ||
| func GetGitHubClient(token string) (*github.Client, error) { |
There was a problem hiding this comment.
Please document all exported types like this one.
There was a problem hiding this comment.
I admit that I should have been documenting all of these as I went, but I did document them all in #11, if that helps.
| // Make a request so we can check that we can connect fine. | ||
| _, res, err := client.RateLimits(ctx) | ||
| if err != nil { | ||
| log.Errorf("Error connecting to GitHub; check your token. Error: %s", err) |
There was a problem hiding this comment.
It is convention to format errors using %v rather than %s
| return client, nil | ||
| } | ||
| func GetJIRAClient(username, password, baseURL string) (*jira.Client, error) { |
| return nil | ||
| } | ||
| type JIRAField struct { |
| return nil | ||
| } | ||
| func compareIssues(ghClient github.Client, jiraClient jira.Client) error { |
There was a problem hiding this comment.
please document this too even though it is not exported. would be nice to know without reading the whole function what this large block of code is trying to accomplish
| } | ||
| func updateIssue(ghIssue github.Issue, jIssue jira.Issue) error { | ||
| log.Debugf("Updating JIRA issue %s with GitHub issue %d", jIssue.ID, *ghIssue.ID) |
There was a problem hiding this comment.
these are currently just placeholders?
There was a problem hiding this comment.
Yeah, they're just to help show that compareIssues is working until the function bodies are added in future PRs.
| return err | ||
| } | ||
| f, err := os.OpenFile(rootCmdCfg.ConfigFileUsed(), os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0751) |
There was a problem hiding this comment.
why 751? the config is not executable. 644 sounds more appropriate.
There was a problem hiding this comment.
Oops. You're right. I got my permission values mixed up.
| rootCmdFile string | ||
| rootCmdCfg *viper.Viper | ||
| since time.Time // The earliest GitHub issue updates we want to retrieve |
There was a problem hiding this comment.
In general we try to have as few global variables as possible. Why are all of these floating around vs being collected in a config?
There was a problem hiding this comment.
Also, by convention we document types and variables by commenting above the variable: https://blog.golang.org/godoc-documenting-go-code
| var ( | ||
| log *logrus.Logger | ||
| log *logrus.Entry |
There was a problem hiding this comment.
Entry is a logger with fields attached, such as the app name that's attached in newLogger().
There was a problem hiding this comment.
Yes I understand that theyre different, what I am asking is why did you change? not a big deal. we typically do not use Entry in other projects but it doesn't at all that it is wrong
There was a problem hiding this comment.
Using an Entry is how it was done in https://github.com/sym3tri/hookz, which is what I was told to use as a template for this project.
squat
commented
Jul 19, 2017
Overall looks ok, Most troubling is the use of lots of global variables rather than collecting all the configuration. |
At this point, all GitHub issues are retrieved, all of their matching JIRA issues are retrieved, and the two are matched in preparation to either create or update the JIRA issues.
Outdated review; the issues have been fixed and approved by another member
Using provided credentials, request a list of GitHub issues updated after a certain time, then the matching list of JIRA issues where they exist. Pair the two lists: for each GitHub issue, if no matching JIRA issue exists, prepare to create it; if an issue exists, prepare to compare the two and update their differences.
Design docs are located at https://docs.google.com/a/coreos.com/document/d/1YjAIqAa-RGC2AZVvlRk1QWbLsJUyt67nIh8lWqKFcK8/edit?usp=sharing.