codereview #5: reverse-map and ImportWarning

Josh Bialkowski created

* Fix git config reader can't read values with spaces
* Add bug-id-revmap config option for the reverse map, and use this
  in the importer
* Add NewImportWarning for things that aren't exactly errors.
  Use this for unhandled changelog events.
* Add NewExportWarning for things that aren't exactly errors.
  Use this for un-exportable status changes.
* Strip newlines from titles on import

Change summary

bridge/core/export.go    | 14 ++++++
bridge/core/import.go    | 15 ++++++
bridge/jira/config.go    |  1 
bridge/jira/export.go    |  2 
bridge/jira/import.go    | 97 ++++++++++++++++++++++++++++-------------
doc/jira_bridge.md       | 15 ++++++
repository/config_git.go |  6 --
7 files changed, 112 insertions(+), 38 deletions(-)

Detailed changes

bridge/core/export.go 🔗

@@ -29,6 +29,7 @@ const (
 
 	// Error happened during export
 	ExportEventError
+	ExportEventWarning
 )
 
 // ExportResult is an event that is emitted during the export process, to
@@ -65,6 +66,11 @@ func (er ExportResult) String() string {
 			return fmt.Sprintf("export error at %s: %s", er.ID, er.Err.Error())
 		}
 		return fmt.Sprintf("export error: %s", er.Err.Error())
+	case ExportEventWarning:
+		if er.ID != "" {
+			return fmt.Sprintf("warning at %s: %s", er.ID, er.Err.Error())
+		}
+		return fmt.Sprintf("warning: %s", er.Err.Error())
 
 	default:
 		panic("unknown export result")
@@ -79,6 +85,14 @@ func NewExportError(err error, id entity.Id) ExportResult {
 	}
 }
 
+func NewExportWarning(err error, id entity.Id) ExportResult {
+	return ExportResult{
+		ID:    id,
+		Err:   err,
+		Event: ExportEventWarning,
+	}
+}
+
 func NewExportNothing(id entity.Id, reason string) ExportResult {
 	return ExportResult{
 		ID:     id,

bridge/core/import.go 🔗

@@ -31,6 +31,7 @@ const (
 
 	// Error happened during import
 	ImportEventError
+	ImportEventWarning
 )
 
 // ImportResult is an event that is emitted during the import process, to
@@ -69,6 +70,12 @@ func (er ImportResult) String() string {
 			return fmt.Sprintf("import error at id %s: %s", er.ID, er.Err.Error())
 		}
 		return fmt.Sprintf("import error: %s", er.Err.Error())
+	case ImportEventWarning:
+		if er.ID != "" {
+			return fmt.Sprintf("warning at id %s: %s", er.ID, er.Err.Error())
+		}
+		return fmt.Sprintf("warning: %s", er.Err.Error())
+
 	default:
 		panic("unknown import result")
 	}
@@ -82,6 +89,14 @@ func NewImportError(err error, id entity.Id) ImportResult {
 	}
 }
 
+func NewImportWarning(err error, id entity.Id) ImportResult {
+	return ImportResult{
+		Err:   err,
+		ID:    id,
+		Event: ImportEventWarning,
+	}
+}
+
 func NewImportNothing(id entity.Id, reason string) ImportResult {
 	return ImportResult{
 		ID:     id,

bridge/jira/config.go 🔗

@@ -26,6 +26,7 @@ const (
 	keyUsername        = "username"
 	keyPassword        = "password"
 	keyIDMap           = "bug-id-map"
+	keyIDRevMap        = "bug-id-revmap"
 	keyCreateDefaults  = "create-issue-defaults"
 	keyCreateGitBug    = "create-issue-gitbug-id"
 

bridge/jira/export.go 🔗

@@ -351,7 +351,7 @@ func (self *jiraExporter) exportBug(
 				exportTime, err = UpdateIssueStatus(client, bugJiraID, jiraStatus)
 				if err != nil {
 					err := errors.Wrap(err, "editing status")
-					out <- core.NewExportError(err, b.Id())
+					out <- core.NewExportWarning(err, b.Id())
 					// Failure to update status isn't necessarily a big error. It's
 					// possible that we just don't have enough information to make that
 					// update. In this case, just don't export the operation.

bridge/jira/import.go 🔗

@@ -207,7 +207,9 @@ func (self *jiraImporter) ensureIssue(
 			return nil, err
 		}
 
-		title := issue.Fields.Summary
+		// NOTE(josh): newlines in titles appears to be rare, but it has been seen
+		// in the wild. It does not appear to be allowed in the JIRA web interface.
+		title := strings.Replace(issue.Fields.Summary, "\n", "", -1)
 		b, _, err = repo.NewBugRaw(
 			author,
 			issue.Fields.Created.Unix(),
@@ -390,7 +392,7 @@ func (self *jiraImporter) ensureChange(
 		return fmt.Errorf("Received changelog entry with no item! (%s)", entry.ID)
 	}
 
-	statusMap, err := getStatusMap(self.conf)
+	statusMap, err := getStatusMapReverse(self.conf)
 	if err != nil {
 		return err
 	}
@@ -423,7 +425,7 @@ func (self *jiraImporter) ensureChange(
 
 		case "status":
 			opr, isRightType := potentialOp.(*bug.SetStatusOperation)
-			if isRightType && statusMap[opr.Status.String()] == item.ToString {
+			if isRightType && statusMap[opr.Status.String()] == item.To {
 				_, err := b.SetMetadata(opr.Id(), map[string]string{
 					keyJiraOperationID: entry.ID,
 				})
@@ -437,7 +439,7 @@ func (self *jiraImporter) ensureChange(
 			// NOTE(josh): JIRA calls it "summary", which sounds more like the body
 			// text, but it's the title
 			opr, isRightType := potentialOp.(*bug.SetTitleOperation)
-			if isRightType && opr.Title == item.ToString {
+			if isRightType && opr.Title == item.To {
 				_, err := b.SetMetadata(opr.Id(), map[string]string{
 					keyJiraOperationID: entry.ID,
 				})
@@ -502,36 +504,42 @@ func (self *jiraImporter) ensureChange(
 			self.out <- core.NewImportLabelChange(op.Id())
 
 		case "status":
-			if statusMap[bug.OpenStatus.String()] == item.ToString {
-				op, err := b.OpenRaw(
-					author,
-					entry.Created.Unix(),
-					map[string]string{
-						keyJiraID:          entry.ID,
-						keyJiraOperationID: derivedID,
-					},
-				)
-				if err != nil {
-					return err
-				}
-				self.out <- core.NewImportStatusChange(op.Id())
-			} else if statusMap[bug.ClosedStatus.String()] == item.ToString {
-				op, err := b.CloseRaw(
-					author,
-					entry.Created.Unix(),
-					map[string]string{
-						keyJiraID:          entry.ID,
-						keyJiraOperationID: derivedID,
-					},
-				)
-				if err != nil {
-					return err
+			statusStr, hasMap := statusMap[item.To]
+			if hasMap {
+				switch statusStr {
+				case bug.OpenStatus.String():
+					op, err := b.OpenRaw(
+						author,
+						entry.Created.Unix(),
+						map[string]string{
+							keyJiraID:          entry.ID,
+							keyJiraOperationID: derivedID,
+						},
+					)
+					if err != nil {
+						return err
+					}
+					self.out <- core.NewImportStatusChange(op.Id())
+
+				case bug.ClosedStatus.String():
+					op, err := b.CloseRaw(
+						author,
+						entry.Created.Unix(),
+						map[string]string{
+							keyJiraID:          entry.ID,
+							keyJiraOperationID: derivedID,
+						},
+					)
+					if err != nil {
+						return err
+					}
+					self.out <- core.NewImportStatusChange(op.Id())
 				}
-				self.out <- core.NewImportStatusChange(op.Id())
 			} else {
 				self.out <- core.NewImportError(
 					fmt.Errorf(
-						"No git-bug status mapped for jira status %s", item.ToString), "")
+						"No git-bug status mapped for jira status %s (%s)",
+						item.ToString, item.To), "")
 			}
 
 		case "summary":
@@ -571,7 +579,9 @@ func (self *jiraImporter) ensureChange(
 			self.out <- core.NewImportCommentEdition(op.Id())
 
 		default:
-			fmt.Printf("Unhandled changelog event %s\n", item.Field)
+			self.out <- core.NewImportWarning(
+				fmt.Errorf(
+					"Unhandled changelog event %s", item.Field), "")
 		}
 
 		// Other Examples:
@@ -599,6 +609,31 @@ func getStatusMap(conf core.Configuration) (map[string]string, error) {
 	return statusMap, err
 }
 
+func getStatusMapReverse(conf core.Configuration) (map[string]string, error) {
+	fwdMap, err := getStatusMap(conf)
+	if err != nil {
+		return fwdMap, err
+	}
+
+	outMap := map[string]string{}
+	for key, val := range fwdMap {
+		outMap[val] = key
+	}
+
+	mapStr, hasConf := conf[keyIDRevMap]
+	if !hasConf {
+		return outMap, nil
+	}
+
+	revMap := make(map[string]string)
+	err = json.Unmarshal([]byte(mapStr), &revMap)
+	for key, val := range revMap {
+		outMap[key] = val
+	}
+
+	return outMap, err
+}
+
 func removeEmpty(values []string) []string {
 	output := make([]string, 0, len(values))
 	for _, value := range values {

doc/jira_bridge.md 🔗

@@ -208,11 +208,24 @@ create-issue-gitbug-id = "customfield_5678"
 You can specify the mapping between `git-bug` status and JIRA status id's using
 the following:
 ```
-bug-id-map = {"open": "1", "closed": "6"}
+bug-id-map = {\"open\": \"1\", \"closed\": \"6\"}
 ```
 
+The format of the map is `<git-bug-status-name>: <jira-status-id>`. In general
+your jira instance will have more statuses than `git-bug` will and you may map
+more than one jira-status to a git-bug status. You can do this with
+`bug-id-revmap`:
+```
+bug-id-revmap = {\"10109\": \"open\", \"10006\": \"open\", \"10814\": \"open\"}
+```
+
+The reverse map `bug-id-revmap` will automatically include the inverse of the
+forward map `bug-id-map`.
+
 Note that in JIRA each different `issuetype` can have a different set of
 statuses. The bridge doesn't currently support more than one mapping, however.
+Also, note that the format of the map is JSON and the git config file syntax
+requires doublequotes to be escaped (as in the examples above).
 
 ### Full example
 

repository/config_git.go 🔗

@@ -66,11 +66,7 @@ func (gc *gitConfig) ReadAll(keyPrefix string) (map[string]string, error) {
 			continue
 		}
 
-		parts := strings.Fields(line)
-		if len(parts) != 2 {
-			return nil, fmt.Errorf("bad git config: %s", line)
-		}
-
+		parts := strings.SplitN(line, " ", 2)
 		result[parts[0]] = parts[1]
 	}