Create a configuration object #13

MorganEPatch · 2017-07-26T22:48:39Z

Depends on #11.

Move the configuration out of global variables and into a configuration object. Also begin moving other utility functions into lib/.

squat

Overall looks good, @LyonesGamer! This is a much needed restructuring. Please re-arrange the remaining functions and then we can merge this.

squat · 2017-08-07T16:33:41Z

cmd/root.go

 // API.
-func createComment(ghComment github.IssueComment, jIssue jira.Issue, ghClient github.Client, jClient jira.Client) error {
-	u, _, err := makeGHRequest(func() (interface{}, *github.Response, error) {
+func createComment(config lib.Config, ghComment github.IssueComment, jIssue jira.Issue, ghClient github.Client, jClient jira.Client) error {


all of these functions that are not directly concerned with configuring and launching the command should be outside of this file and package. this function should be in lib.

squat · 2017-08-07T16:33:51Z

cmd/root.go

 // updateComment compares the body of a GitHub comment with the body (minus header)
 // of the JIRA comment, and updates the JIRA comment if necessary.
-func updateComment(ghComment github.IssueComment, jComment jira.Comment, jIssue jira.Issue, ghClient github.Client, jClient jira.Client) error {
+func updateComment(config lib.Config, ghComment github.IssueComment, jComment jira.Comment, jIssue jira.Issue, ghClient github.Client, jClient jira.Client) error {


same: all of these functions that are not directly concerned with configuring and launching the command should be outside of this file and package. this function should be in lib.

squat · 2017-08-07T16:34:00Z

cmd/root.go

 // matches each one to a comment in `existing`. If it finds a match, it calls
 // updateComment; if it doesn't, it calls createComment.
-func createComments(ghIssue github.Issue, jIssue jira.Issue, existing []jira.Comment, ghClient github.Client, jClient jira.Client) error {
+func createComments(config lib.Config, ghIssue github.Issue, jIssue jira.Issue, existing []jira.Comment, ghClient github.Client, jClient jira.Client) error {


same: all of these functions that are not directly concerned with configuring and launching the command should be outside of this file and package. this function should be in lib.

squat · 2017-08-07T16:34:10Z

cmd/root.go

 // createIssue generates a JIRA issue from the various fields on the given GitHub issue, then
 // sends it to the JIRA API.
-func createIssue(issue github.Issue, ghClient github.Client, jClient jira.Client) error {
+func createIssue(config lib.Config, issue github.Issue, ghClient github.Client, jClient jira.Client) error {


same: all of these functions that are not directly concerned with configuring and launching the command should be outside of this file and package. this function should be in lib.

squat · 2017-08-07T16:34:24Z

cmd/root.go

 // differ, the differing fields of the JIRA issue are updated to match the GitHub
 // issue.
-func updateIssue(ghIssue github.Issue, jIssue jira.Issue, ghClient github.Client, jClient jira.Client) error {
+func updateIssue(config lib.Config, ghIssue github.Issue, jIssue jira.Issue, ghClient github.Client, jClient jira.Client) error {


same: all of these functions that are not directly concerned with configuring and launching the command should be outside of this file and package. this function should be in lib.

squat · 2017-08-07T16:34:29Z

cmd/root.go

 // then matches each one. If a JIRA issue already exists for a given GitHub issue,
 // it calls updateIssue; if no JIRA issue already exists, it calls createIssue.
-func compareIssues(ghClient github.Client, jiraClient jira.Client) error {
+func compareIssues(config lib.Config, ghClient github.Client, jiraClient jira.Client) error {


all of these functions that are not directly concerned with configuring and launching the command should be outside of this file and package. this function should be in lib.

squat · 2017-08-07T16:36:19Z

cmd/root.go

 // then makes an API request to confirm that the service is running and the auth token
 // is valid
-func getGitHubClient(token string) (*github.Client, error) {
+func getGitHubClient(config lib.Config) (*github.Client, error) {


this should be part of the the clients file. lib should know how to create a new github client given a config.

This should have been moved to the clients file. Some weird issue must have happened during rebasing or something...

squat · 2017-08-07T16:36:42Z

cmd/root.go

+//
+// The validity of the client and its authentication are not checked here. One way
+// to check them would be to call config.LoadJIRAConfig() after this function.
+func getJIRAClient(config lib.Config) (*jira.Client, error) {


this should be part of the the clients file. lib should know how to create a new jira client given a config.

MorganEPatch · 2017-08-07T17:55:46Z

@squat Moving all of the functions into separate packages occurs in #14.

squat

This repo is starting to look much better. Just a few small comments.

squat · 2017-08-07T22:25:11Z

lib/clients.go

+
+// GetGitHubClient initializes a GitHub API client with an OAuth client for authentication,
+// then makes an API request to confirm that the service is running and the auth token
+// is valid


add a period here.

squat · 2017-08-07T22:29:27Z

cmd/root.go


 // Execute provides a single function to run the root command and handle errors.
 func Execute() {
+	// Create a temporary logger that we can use if an error occurs before the real one is instantiated


add a period e here.

squat · 2017-08-07T22:30:26Z

cmd/root.go

 		found := false
 		for _, jIssue := range jiraIssues {
-			id, _ := jIssue.Fields.Unknowns.Int(fmt.Sprintf("customfield_%s", ghIDFieldID))
+			id, _ := jIssue.Fields.Unknowns.Int(config.GetFieldKey(lib.GitHubID))


is this suppressing an error?

It's suppressing the error that the issue doesn't have the GitHub ID set; that can't happen since we filtered it by the GitHub ID. I'll add an error handler though, I suppose weird API errors are never unheard of.

squat · 2017-08-07T22:32:43Z

lib/config.go

+	lastUpdate     string
+}
+
+// Config is the root configuration object the application creates


add a period here too.

squat · 2017-08-07T22:33:01Z

lib/config.go

+
+// Config is the root configuration object the application creates
+type Config struct {
+	// cmdFile is the file Viper is using for its configuration (default $HOME/.issue-sync.json)


make all of these comments full sentences please

squat · 2017-08-07T22:33:12Z

lib/config.go

+
+// NewConfig creates a new, immutable configuration object. This object
+// holds the Viper configuration and the logger, and is validated. The
+// JIRA configuration is not yet initialized


MorganEPatch · 2017-08-07T23:02:58Z

@squat Fixed.

Create a new Config object which represents the current application configuration. It stores both the viper configuration and computed configuration values such as the custom field IDs. Also, move API client related functions into a new lib file to make them accessible to the whole application without circular imports.

MorganEPatch self-assigned this Jul 26, 2017

MorganEPatch mentioned this pull request Aug 1, 2017

Move functionality to lib/ files #14

Merged

squat suggested changes Aug 7, 2017

View reviewed changes

squat approved these changes Aug 7, 2017

View reviewed changes

MorganEPatch merged commit ff3af54 into coreos:master Aug 7, 2017

Create a configuration object #13

Create a configuration object #13

Conversation

MorganEPatch commented Jul 26, 2017

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorganEPatch commented Aug 7, 2017

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorganEPatch commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants