codereview #2: some cleanup, correct use of nothing-events

Josh Bialkowski created

* return error, don't panic
* skipping status export is an error
* use switch in config.go
* move PromptPassword to input
* move client construction into getIdentityClient
* use non-pointer context throughout client since it is an interface
* remove some TODOs
* don't emit multiple nothing-events, just one per bug only if nothing
  happened.
* rename EditBody to EditCreateComment
* add configuration notes about additional values
* store bug id map in a dictionary in the config
* some fixes from testing

Change summary

bridge/jira/client.go  |  35 ++++++------
bridge/jira/config.go  |  56 +++++--------------
bridge/jira/export.go  | 124 +++++++++++++++++++++++--------------------
bridge/jira/import.go  |  77 +++++++++++---------------
bug/op_edit_comment.go |  17 +++--
cache/bug_cache.go     |   8 +-
doc/jira_bridge.md     |   3 
input/prompt.go        |  36 ++++++++++++
8 files changed, 183 insertions(+), 173 deletions(-)

Detailed changes

bridge/jira/client.go 🔗

@@ -15,6 +15,7 @@ import (
 
 	"github.com/MichaelMure/git-bug/bridge/core"
 	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/input"
 	"github.com/pkg/errors"
 )
 
@@ -318,12 +319,12 @@ func (self *ClientTransport) RoundTrip(
 type Client struct {
 	*http.Client
 	serverURL string
-	ctx       *context.Context
+	ctx       context.Context
 }
 
 // NewClient Construct a new client connected to the provided server and
 // utilizing the given context for asynchronous events
-func NewClient(serverURL string, ctx *context.Context) *Client {
+func NewClient(serverURL string, ctx context.Context) *Client {
 	cookiJar, _ := cookiejar.New(nil)
 	client := &http.Client{
 		Transport: &ClientTransport{underlyingTransport: http.DefaultTransport},
@@ -353,7 +354,7 @@ func (client *Client) Login(conf core.Configuration) error {
 	password := conf[keyPassword]
 	if password == "" {
 		var err error
-		password, err = PromptPassword()
+		password, err = input.PromptPassword()
 		if err != nil {
 			return err
 		}
@@ -397,7 +398,7 @@ func (client *Client) RefreshTokenRaw(credentialsJSON []byte) error {
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		req = req.WithContext(ctx)
 	}
@@ -465,7 +466,7 @@ func (client *Client) Search(jql string, maxResults int, startAt int) (
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -593,7 +594,7 @@ func (client *Client) GetIssue(
 	request.URL.RawQuery = query.Encode()
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -648,7 +649,7 @@ func (client *Client) GetComments(
 	request.URL.RawQuery = query.Encode()
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -776,7 +777,7 @@ func (client *Client) GetChangeLog(
 	request.URL.RawQuery = query.Encode()
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -905,7 +906,7 @@ func (client *Client) GetProject(projectIDOrKey string) (*Project, error) {
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -972,7 +973,7 @@ func (client *Client) CreateIssue(
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1086,7 +1087,7 @@ func (client *Client) UpdateIssueBody(
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1138,7 +1139,7 @@ func (client *Client) AddComment(issueKeyOrID, body string) (*Comment, error) {
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1189,7 +1190,7 @@ func (client *Client) UpdateComment(issueKeyOrID, commentID, body string) (
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1255,7 +1256,7 @@ func (client *Client) UpdateLabels(
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1304,7 +1305,7 @@ func (client *Client) GetTransitions(issueKeyOrID string) (
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1368,7 +1369,7 @@ func (client *Client) DoTransition(
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}
@@ -1414,7 +1415,7 @@ func (client *Client) GetServerInfo() (*ServerInfo, error) {
 	}
 
 	if client.ctx != nil {
-		ctx, cancel := context.WithTimeout(*client.ctx, defaultTimeout)
+		ctx, cancel := context.WithTimeout(client.ctx, defaultTimeout)
 		defer cancel()
 		request = request.WithContext(ctx)
 	}

bridge/jira/config.go 🔗

@@ -8,15 +8,13 @@ import (
 	"os"
 	"strconv"
 	"strings"
-	"syscall"
 	"time"
 
 	"github.com/pkg/errors"
-	"golang.org/x/crypto/ssh/terminal"
 
 	"github.com/MichaelMure/git-bug/bridge/core"
+	"github.com/MichaelMure/git-bug/input"
 	"github.com/MichaelMure/git-bug/repository"
-	"github.com/MichaelMure/git-bug/util/interrupt"
 )
 
 const (
@@ -26,14 +24,20 @@ const (
 	keyCredentialsFile = "credentials-file"
 	keyUsername        = "username"
 	keyPassword        = "password"
-	keyMapOpenID       = "bug-open-id"
-	keyMapCloseID      = "bug-closed-id"
+	keyIDMap           = "bug-id-map"
 	keyCreateDefaults  = "create-issue-defaults"
 	keyCreateGitBug    = "create-issue-gitbug-id"
 
 	defaultTimeout = 60 * time.Second
 )
 
+const moreConfigText = `
+NOTE: There are a few optional configuration values that you can additionally
+set in your git configuration to influence the behavior of the bridge. Please
+see the notes at:
+https://github.com/MichaelMure/git-bug/blob/master/doc/jira_bridge.md
+`
+
 // Configure sets up the bridge configuration
 func (g *Jira) Configure(
 	repo repository.RepoCommon, params core.BridgeParams) (
@@ -91,7 +95,7 @@ func (g *Jira) Configure(
 		return nil, err
 	}
 
-	password, err = PromptPassword()
+	password, err = input.PromptPassword()
 	if err != nil {
 		return nil, err
 	}
@@ -121,24 +125,27 @@ func (g *Jira) Configure(
 	conf[core.KeyTarget] = target
 	conf[keyServer] = serverURL
 	conf[keyProject] = project
-	if choice == 1 {
+	switch choice {
+	case 1:
 		conf[keyCredentialsFile] = credentialsFile
 		err = ioutil.WriteFile(credentialsFile, jsonData, 0644)
 		if err != nil {
 			return nil, errors.Wrap(
 				err, fmt.Sprintf("Unable to write credentials to %s", credentialsFile))
 		}
-	} else if choice == 2 {
+	case 2:
 		conf[keyUsername] = username
 		conf[keyPassword] = password
-	} else if choice == 3 {
+	case 3:
 		conf[keyUsername] = username
 	}
+
 	err = g.ValidateConfig(conf)
 	if err != nil {
 		return nil, err
 	}
 
+	fmt.Print(moreConfigText)
 	return conf, nil
 }
 
@@ -209,34 +216,3 @@ func prompt(description, name string) (string, error) {
 		return line, nil
 	}
 }
-
-// PromptPassword performs interactive input collection to get the user password
-func PromptPassword() (string, error) {
-	termState, err := terminal.GetState(int(syscall.Stdin))
-	if err != nil {
-		return "", err
-	}
-
-	cancel := interrupt.RegisterCleaner(func() error {
-		return terminal.Restore(int(syscall.Stdin), termState)
-	})
-	defer cancel()
-
-	for {
-		fmt.Print("password: ")
-		bytePassword, err := terminal.ReadPassword(int(syscall.Stdin))
-		// new line for coherent formatting, ReadPassword clip the normal new line
-		// entered by the user
-		fmt.Println()
-
-		if err != nil {
-			return "", err
-		}
-
-		if len(bytePassword) > 0 {
-			return string(bytePassword), nil
-		}
-
-		fmt.Println("password is empty")
-	}
-}

bridge/jira/export.go 🔗

@@ -15,10 +15,19 @@ import (
 	"github.com/MichaelMure/git-bug/entity"
 )
 
+var (
+	ErrMissingCredentials = errors.New("missing credentials")
+)
+
 // jiraExporter implement the Exporter interface
 type jiraExporter struct {
 	conf core.Configuration
 
+	// the current user identity
+	// NOTE: this is only needed to mock the credentials database in
+	// getIdentityClient
+	userIdentity entity.Id
+
 	// cache identities clients
 	identityClient map[entity.Id]*Client
 
@@ -36,7 +45,6 @@ type jiraExporter struct {
 // Init .
 func (self *jiraExporter) Init(conf core.Configuration) error {
 	self.conf = conf
-	//TODO: initialize with multiple tokens
 	self.identityClient = make(map[entity.Id]*Client)
 	self.cachedOperationIDs = make(map[entity.Id]string)
 	self.cachedLabels = make(map[string]string)
@@ -47,17 +55,27 @@ func (self *jiraExporter) Init(conf core.Configuration) error {
 // of the given identity. If no client were found it will initialize it from
 // the known credentials map and cache it for next use
 func (self *jiraExporter) getIdentityClient(
-	ctx *context.Context, id entity.Id) (*Client, error) {
+	ctx context.Context, id entity.Id) (*Client, error) {
 	client, ok := self.identityClient[id]
 	if ok {
 		return client, nil
 	}
 
-	// TODO(josh)[]: The github exporter appears to contain code that will
-	// allow it to export bugs owned by other people as long as we have a token
-	// for that identity. I guess the equivalent for us will be as long as we
-	// have a credentials pair for that identity.
-	return nil, fmt.Errorf("Not implemented")
+	client = NewClient(self.conf[keyServer], ctx)
+
+	// NOTE: as a future enhancement, the bridge would ideally be able to generate
+	// a separate session token for each user that we have stored credentials
+	// for. However we currently only support a single user.
+	if id != self.userIdentity {
+		return nil, ErrMissingCredentials
+	}
+	err := client.Login(self.conf)
+	if err != nil {
+		return nil, err
+	}
+
+	self.identityClient[id] = client
+	return client, nil
 }
 
 // ExportAll export all event made by the current user to Jira
@@ -72,19 +90,10 @@ func (self *jiraExporter) ExportAll(
 		return nil, err
 	}
 
-	// TODO(josh)[]: The github exporter appears to contain code that will
-	// allow it to export bugs owned by other people as long as we have a token
-	// for that identity. I guess the equivalent for us will be as long as we
-	// have a credentials pair for that identity.
-	client := NewClient(self.conf[keyServer], &ctx)
-	err = client.Login(self.conf)
-	self.identityClient[user.Id()] = client
-
-	if err != nil {
-		return nil, err
-	}
-
-	client, err = self.getIdentityClient(&ctx, user.Id())
+	// NOTE: this is currently only need to mock the credentials database in
+	// getIdentityClient.
+	self.userIdentity = user.Id()
+	client, err := self.getIdentityClient(ctx, user.Id())
 	if err != nil {
 		return nil, err
 	}
@@ -129,7 +138,11 @@ func (self *jiraExporter) ExportAll(
 
 				if snapshot.HasAnyActor(allIdentitiesIds...) {
 					// try to export the bug and it associated events
-					self.exportBug(ctx, b, since, out)
+					err := self.exportBug(ctx, b, since, out)
+					if err != nil {
+						out <- core.NewExportError(errors.Wrap(err, "can't export bug"), id)
+						return
+					}
 				} else {
 					out <- core.NewExportNothing(id, "not an actor")
 				}
@@ -143,7 +156,7 @@ func (self *jiraExporter) ExportAll(
 // exportBug publish bugs and related events
 func (self *jiraExporter) exportBug(
 	ctx context.Context, b *cache.BugCache, since time.Time,
-	out chan<- core.ExportResult) {
+	out chan<- core.ExportResult) error {
 	snapshot := b.Snapshot()
 
 	var bugJiraID string
@@ -162,7 +175,7 @@ func (self *jiraExporter) exportBug(
 	if ok && origin != target {
 		out <- core.NewExportNothing(
 			b.Id(), fmt.Sprintf("issue tagged with origin: %s", origin))
-		return
+		return nil
 	}
 
 	// skip bug if it is a jira bug but is associated with another project
@@ -171,25 +184,24 @@ func (self *jiraExporter) exportBug(
 	if ok && !stringInSlice(project, []string{self.project.ID, self.project.Key}) {
 		out <- core.NewExportNothing(
 			b.Id(), fmt.Sprintf("issue tagged with project: %s", project))
-		return
+		return nil
 	}
 
 	// get jira bug ID
 	jiraID, ok := snapshot.GetCreateMetadata(keyJiraID)
 	if ok {
-		out <- core.NewExportNothing(b.Id(), "bug creation already exported")
 		// will be used to mark operation related to a bug as exported
 		bugJiraID = jiraID
 	} else {
 		// check that we have credentials for operation author
-		client, err := self.getIdentityClient(&ctx, author.Id())
+		client, err := self.getIdentityClient(ctx, author.Id())
 		if err != nil {
 			// if bug is not yet exported and we do not have the author's credentials
 			// then there is nothing we can do, so just skip this bug
 			out <- core.NewExportNothing(
 				b.Id(), fmt.Sprintf("missing author token for user %.8s",
 					author.Id().String()))
-			return
+			return err
 		}
 
 		// Load any custom fields required to create an issue from the git
@@ -199,13 +211,15 @@ func (self *jiraExporter) exportBug(
 		if hasConf {
 			err = json.Unmarshal([]byte(defaultFields), &fields)
 			if err != nil {
-				panic("Invalid JSON in config")
+				return err
 			}
 		} else {
 			// If there is no configuration provided, at the very least the
 			// "issueType" field is always required. 10001 is "story" which I'm
 			// pretty sure is standard/default on all JIRA instances.
-			fields["issueType"] = "10001"
+			fields["issuetype"] = map[string]interface{}{
+				"id": "10001",
+			}
 		}
 		bugIDField, hasConf := self.conf[keyCreateGitBug]
 		if hasConf {
@@ -220,7 +234,7 @@ func (self *jiraExporter) exportBug(
 		if err != nil {
 			err := errors.Wrap(err, "exporting jira issue")
 			out <- core.NewExportError(err, b.Id())
-			return
+			return err
 		}
 
 		id := result.ID
@@ -231,7 +245,7 @@ func (self *jiraExporter) exportBug(
 		if err != nil {
 			err := errors.Wrap(err, "marking operation as exported")
 			out <- core.NewExportError(err, b.Id())
-			return
+			return err
 		}
 
 		// commit operation to avoid creating multiple issues with multiple pushes
@@ -239,7 +253,7 @@ func (self *jiraExporter) exportBug(
 		if err != nil {
 			err := errors.Wrap(err, "bug commit")
 			out <- core.NewExportError(err, b.Id())
-			return
+			return err
 		}
 
 		// cache bug jira ID
@@ -250,7 +264,10 @@ func (self *jiraExporter) exportBug(
 	self.cachedOperationIDs[createOp.Id()] = bugJiraID
 
 	// lookup the mapping from git-bug "status" to JIRA "status" id
-	statusMap := getStatusMap(self.conf)
+	statusMap, err := getStatusMap(self.conf)
+	if err != nil {
+		return err
+	}
 
 	for _, op := range snapshot.Operations[1:] {
 		// ignore SetMetadata operations
@@ -263,17 +280,15 @@ func (self *jiraExporter) exportBug(
 		// Jira
 		if id, ok := op.GetMetadata(keyJiraID); ok {
 			self.cachedOperationIDs[op.Id()] = id
-			out <- core.NewExportNothing(op.Id(), "already exported operation")
 			continue
 		}
 
 		opAuthor := op.GetAuthor()
-		client, err := self.getIdentityClient(&ctx, opAuthor.Id())
+		client, err := self.getIdentityClient(ctx, opAuthor.Id())
 		if err != nil {
-			out <- core.NewExportNothing(
-				op.Id(), fmt.Sprintf(
-					"missing operation author credentials for user %.8s",
-					author.Id().String()))
+			out <- core.NewExportError(
+				fmt.Errorf("missing operation author credentials for user %.8s",
+					author.Id().String()), op.Id())
 			continue
 		}
 
@@ -285,7 +300,7 @@ func (self *jiraExporter) exportBug(
 			if err != nil {
 				err := errors.Wrap(err, "adding comment")
 				out <- core.NewExportError(err, b.Id())
-				return
+				return err
 			}
 			id = comment.ID
 			out <- core.NewExportComment(op.Id())
@@ -301,7 +316,7 @@ func (self *jiraExporter) exportBug(
 				if err != nil {
 					err := errors.Wrap(err, "editing issue")
 					out <- core.NewExportError(err, b.Id())
-					return
+					return err
 				}
 				out <- core.NewExportCommentEdition(op.Id())
 				id = bugJiraID
@@ -319,7 +334,7 @@ func (self *jiraExporter) exportBug(
 				if err != nil {
 					err := errors.Wrap(err, "editing comment")
 					out <- core.NewExportError(err, b.Id())
-					return
+					return err
 				}
 				out <- core.NewExportCommentEdition(op.Id())
 				// JIRA doesn't track all comment edits, they will only tell us about
@@ -343,13 +358,10 @@ func (self *jiraExporter) exportBug(
 					continue
 				}
 				out <- core.NewExportStatusChange(op.Id())
-				// TODO(josh)[c2c6767]: query changelog to get the changelog-id so that
-				// we don't re-import the same change.
 				id = bugJiraID
 			} else {
-				out <- core.NewExportNothing(
-					op.Id(), fmt.Sprintf(
-						"No jira status mapped for %.8s", opr.Status.String()))
+				out <- core.NewExportError(fmt.Errorf(
+					"No jira status mapped for %.8s", opr.Status.String()), b.Id())
 			}
 
 		case *bug.SetTitleOperation:
@@ -357,11 +369,9 @@ func (self *jiraExporter) exportBug(
 			if err != nil {
 				err := errors.Wrap(err, "editing title")
 				out <- core.NewExportError(err, b.Id())
-				return
+				return err
 			}
 			out <- core.NewExportTitleEdition(op.Id())
-			// TODO(josh)[c2c6767]: query changelog to get the changelog-id so that
-			// we don't re-import the same change.
 			id = bugJiraID
 
 		case *bug.LabelChangeOperation:
@@ -370,11 +380,9 @@ func (self *jiraExporter) exportBug(
 			if err != nil {
 				err := errors.Wrap(err, "updating labels")
 				out <- core.NewExportError(err, b.Id())
-				return
+				return err
 			}
 			out <- core.NewExportLabelChange(op.Id())
-			// TODO(josh)[c2c6767]: query changelog to get the changelog-id so that
-			// we don't re-import the same change.
 			id = bugJiraID
 
 		default:
@@ -382,16 +390,12 @@ func (self *jiraExporter) exportBug(
 		}
 
 		// mark operation as exported
-		// TODO(josh)[c2c6767]: Should we query the changelog after we export?
-		// Some of the operations above don't record an ID... so we are bound to
-		// re-import them. It shouldn't cause too much of an issue but we will have
-		// duplicate edit entries for everything and it would be nice to avoid that.
 		err = markOperationAsExported(
 			b, op.Id(), id, self.project.Key, exportTime)
 		if err != nil {
 			err := errors.Wrap(err, "marking operation as exported")
 			out <- core.NewExportError(err, b.Id())
-			return
+			return err
 		}
 
 		// commit at each operation export to avoid exporting same events multiple
@@ -400,9 +404,11 @@ func (self *jiraExporter) exportBug(
 		if err != nil {
 			err := errors.Wrap(err, "bug commit")
 			out <- core.NewExportError(err, b.Id())
-			return
+			return err
 		}
 	}
+
+	return nil
 }
 
 func markOperationAsExported(

bridge/jira/import.go 🔗

@@ -2,6 +2,7 @@ package jira
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"sort"
@@ -57,7 +58,7 @@ func (self *jiraImporter) ImportAll(
 	go func() {
 		defer close(self.out)
 
-		client := NewClient(serverURL, &ctx)
+		client := NewClient(serverURL, ctx)
 		err := client.Login(self.conf)
 		if err != nil {
 			out <- core.NewImportError(err, "")
@@ -140,7 +141,9 @@ func (self *jiraImporter) ImportAll(
 				out <- core.NewImportError(changelogIter.Err, "")
 			}
 
-			if err := bug.CommitAsNeeded(); err != nil {
+			if !bug.NeedCommit() {
+				out <- core.NewImportNothing(bug.Id(), "no imported operation")
+			} else if err := bug.Commit(); err != nil {
 				err = fmt.Errorf("bug commit: %v", err)
 				out <- core.NewImportError(err, "")
 				return
@@ -194,9 +197,6 @@ func (self *jiraImporter) ensureIssue(
 		return nil, err
 	}
 
-	// TODO(josh)[f8808eb]: Consider looking up the git-bug entry directly from
-	// the jira field which contains it, if we have a custom field configured
-	// to store git-bug IDs.
 	b, err := repo.ResolveBugCreateMetadata(keyJiraID, issue.ID)
 	if err != nil && err != bug.ErrBugNotExist {
 		return nil, err
@@ -226,8 +226,6 @@ func (self *jiraImporter) ensureIssue(
 		}
 
 		self.out <- core.NewImportBug(b.Id())
-	} else {
-		self.out <- core.NewImportNothing("", "bug already imported")
 	}
 
 	return b, nil
@@ -249,9 +247,7 @@ func (self *jiraImporter) ensureComment(
 
 	targetOpID, err := b.ResolveOperationWithMetadata(
 		keyJiraID, item.ID)
-	if err == nil {
-		self.out <- core.NewImportNothing("", "comment already imported")
-	} else if err != cache.ErrNoMatchingOp {
+	if err != nil && err != cache.ErrNoMatchingOp {
 		return err
 	}
 
@@ -298,9 +294,7 @@ func (self *jiraImporter) ensureComment(
 	derivedID := getTimeDerivedID(item.ID, item.Updated)
 	_, err = b.ResolveOperationWithMetadata(
 		keyJiraID, derivedID)
-	if err == nil {
-		self.out <- core.NewImportNothing("", "update already imported")
-	} else if err != cache.ErrNoMatchingOp {
+	if err != nil && err != cache.ErrNoMatchingOp {
 		return err
 	}
 
@@ -373,8 +367,6 @@ func (self *jiraImporter) ensureChange(
 	// operation and we've already done the match, so we skip this one
 	_, err := b.ResolveOperationWithMetadata(keyJiraOperationID, entry.ID)
 	if err == nil {
-		self.out <- core.NewImportNothing(
-			"", "changelog entry already matched to export")
 		return nil
 	} else if err != cache.ErrNoMatchingOp {
 		return err
@@ -395,7 +387,10 @@ func (self *jiraImporter) ensureChange(
 		return fmt.Errorf("Received changelog entry with no item! (%s)", entry.ID)
 	}
 
-	statusMap := getStatusMap(self.conf)
+	statusMap, err := getStatusMap(self.conf)
+	if err != nil {
+		return err
+	}
 
 	// NOTE(josh): first do an initial scan and see if any of the changed items
 	// matches the current potential operation. If it does, then we know that this
@@ -405,8 +400,6 @@ func (self *jiraImporter) ensureChange(
 	for _, item := range entry.Items {
 		switch item.Field {
 		case "labels":
-			// TODO(josh)[d7fd71c]: move set-symmetric-difference code to a helper
-			// function. Probably in jira.go or something.
 			fromLabels := strings.Split(item.FromString, " ")
 			toLabels := strings.Split(item.ToString, " ")
 			removedLabels, addedLabels, _ := setSymmetricDifference(
@@ -416,10 +409,12 @@ func (self *jiraImporter) ensureChange(
 			if isRightType &&
 				labelSetsMatch(addedLabels, opr.Added) &&
 				labelSetsMatch(removedLabels, opr.Removed) {
-				b.SetMetadata(opr.Id(), map[string]string{
+				_, err := b.SetMetadata(opr.Id(), map[string]string{
 					keyJiraOperationID: entry.ID,
 				})
-				self.out <- core.NewImportNothing("", "matched export")
+				if err != nil {
+					return err
+				}
 				return nil
 			}
 
@@ -430,9 +425,8 @@ func (self *jiraImporter) ensureChange(
 					keyJiraOperationID: entry.ID,
 				})
 				if err != nil {
-					panic("Can't set metadata")
+					return err
 				}
-				self.out <- core.NewImportNothing("", "matched export")
 				return nil
 			}
 
@@ -445,9 +439,8 @@ func (self *jiraImporter) ensureChange(
 					keyJiraOperationID: entry.ID,
 				})
 				if err != nil {
-					panic("Can't set metadata")
+					return err
 				}
-				self.out <- core.NewImportNothing("", "matched export")
 				return nil
 			}
 
@@ -462,9 +455,8 @@ func (self *jiraImporter) ensureChange(
 					keyJiraOperationID: entry.ID,
 				})
 				if err != nil {
-					panic("Can't set metadata")
+					return err
 				}
-				self.out <- core.NewImportNothing("", "matched export")
 				return nil
 			}
 		}
@@ -477,7 +469,6 @@ func (self *jiraImporter) ensureChange(
 		derivedID := getIndexDerivedID(entry.ID, idx)
 		_, err := b.ResolveOperationWithMetadata(keyJiraOperationID, derivedID)
 		if err == nil {
-			self.out <- core.NewImportNothing("", "update already imported")
 			continue
 		} else if err != cache.ErrNoMatchingOp {
 			return err
@@ -485,8 +476,6 @@ func (self *jiraImporter) ensureChange(
 
 		switch item.Field {
 		case "labels":
-			// TODO(josh)[d7fd71c]: move set-symmetric-difference code to a helper
-			// function. Probably in jira.go or something.
 			fromLabels := strings.Split(item.FromString, " ")
 			toLabels := strings.Split(item.ToString, " ")
 			removedLabels, addedLabels, _ := setSymmetricDifference(
@@ -541,9 +530,9 @@ func (self *jiraImporter) ensureChange(
 				}
 				self.out <- core.NewImportStatusChange(op.Id())
 			} else {
-				self.out <- core.NewImportNothing(
-					"", fmt.Sprintf(
-						"No git-bug status mapped for jira status %s", item.ToString))
+				self.out <- core.NewImportError(
+					fmt.Errorf(
+						"No git-bug status mapped for jira status %s", item.ToString), "")
 			}
 
 		case "summary":
@@ -568,7 +557,7 @@ func (self *jiraImporter) ensureChange(
 		case "description":
 			// NOTE(josh): JIRA calls it "description", which sounds more like the
 			// title but it's actually the body
-			op, err := b.EditBodyRaw(
+			op, err := b.EditCreateCommentRaw(
 				author,
 				entry.Created.Unix(),
 				string(item.ToString),
@@ -596,18 +585,16 @@ func (self *jiraImporter) ensureChange(
 	return nil
 }
 
-func getStatusMap(conf core.Configuration) map[string]string {
-	var hasConf bool
-	statusMap := make(map[string]string)
-	statusMap[bug.OpenStatus.String()], hasConf = conf[keyMapOpenID]
+func getStatusMap(conf core.Configuration) (map[string]string, error) {
+	mapStr, hasConf := conf[keyIDMap]
 	if !hasConf {
-		// Default to "1" which is the built-in jira "Open" status
-		statusMap[bug.OpenStatus.String()] = "1"
+		return map[string]string{
+			bug.OpenStatus.String():   "1",
+			bug.ClosedStatus.String(): "6",
+		}, nil
 	}
-	statusMap[bug.ClosedStatus.String()], hasConf = conf[keyMapCloseID]
-	if !hasConf {
-		// Default to "6" which is the built-in jira "Closed" status
-		statusMap[bug.OpenStatus.String()] = "6"
-	}
-	return statusMap
+
+	statusMap := make(map[string]string)
+	err := json.Unmarshal([]byte(mapStr), &statusMap)
+	return statusMap, err
 }

bug/op_edit_comment.go 🔗

@@ -148,12 +148,6 @@ func EditComment(b Interface, author identity.Interface, unixTime int64, target
 	return EditCommentWithFiles(b, author, unixTime, target, message, nil)
 }
 
-// Convenience function to edit the body of a bug (the first comment)
-func EditBody(b Interface, author identity.Interface, unixTime int64, message string) (*EditCommentOperation, error) {
-	createOp := b.FirstOp().(*CreateOperation)
-	return EditComment(b, author, unixTime, createOp.Id(), message)
-}
-
 func EditCommentWithFiles(b Interface, author identity.Interface, unixTime int64, target entity.Id, message string, files []git.Hash) (*EditCommentOperation, error) {
 	editCommentOp := NewEditCommentOp(author, unixTime, target, message, files)
 	if err := editCommentOp.Validate(); err != nil {
@@ -163,3 +157,14 @@ func EditCommentWithFiles(b Interface, author identity.Interface, unixTime int64
 	return editCommentOp, nil
 }
 
+// Convenience function to edit the body of a bug (the first comment)
+func EditCreateComment(b Interface, author identity.Interface, unixTime int64, message string) (*EditCommentOperation, error) {
+	createOp := b.FirstOp().(*CreateOperation)
+	return EditComment(b, author, unixTime, createOp.Id(), message)
+}
+
+// Convenience function to edit the body of a bug (the first comment)
+func EditCreateCommentWithFiles(b Interface, author identity.Interface, unixTime int64, message string, files []git.Hash) (*EditCommentOperation, error) {
+	createOp := b.FirstOp().(*CreateOperation)
+	return EditCommentWithFiles(b, author, unixTime, createOp.Id(), message, files)
+}

cache/bug_cache.go 🔗

@@ -210,17 +210,17 @@ func (c *BugCache) SetTitleRaw(author *IdentityCache, unixTime int64, title stri
 	return op, c.notifyUpdated()
 }
 
-func (c *BugCache) EditBody(body string) (*bug.EditCommentOperation, error) {
+func (c *BugCache) EditCreateComment(body string) (*bug.EditCommentOperation, error) {
 	author, err := c.repoCache.GetUserIdentity()
 	if err != nil {
 		return nil, err
 	}
 
-	return c.EditBodyRaw(author, time.Now().Unix(), body, nil)
+	return c.EditCreateCommentRaw(author, time.Now().Unix(), body, nil)
 }
 
-func (c *BugCache) EditBodyRaw(author *IdentityCache, unixTime int64, body string, metadata map[string]string) (*bug.EditCommentOperation, error) {
-	op, err := bug.EditBody(c.bug, author.Identity, unixTime, body)
+func (c *BugCache) EditCreateCommentRaw(author *IdentityCache, unixTime int64, body string, metadata map[string]string) (*bug.EditCommentOperation, error) {
+	op, err := bug.EditCreateComment(c.bug, author.Identity, unixTime, body)
 	if err != nil {
 		return nil, err
 	}

doc/jira_bridge.md 🔗

@@ -208,8 +208,7 @@ create-issue-gitbug-id = "customfield_5678"
 You can specify the mapping between `git-bug` status and JIRA status id's using
 the following:
 ```
-bug-open-id = 1
-bug-closed-id = 6
+bug-id-map = {"open": "1", "closed": "6"}
 ```
 
 Note that in JIRA each different `issuetype` can have a different set of

input/prompt.go 🔗

@@ -5,6 +5,10 @@ import (
 	"fmt"
 	"os"
 	"strings"
+	"syscall"
+
+	"github.com/MichaelMure/git-bug/util/interrupt"
+	"golang.org/x/crypto/ssh/terminal"
 )
 
 func PromptValue(name string, preValue string) (string, error) {
@@ -42,3 +46,35 @@ func promptValue(name string, preValue string, required bool) (string, error) {
 		return line, nil
 	}
 }
+
+// PromptPassword performs interactive input collection to get the user password
+// while halting echo on the controlling terminal.
+func PromptPassword() (string, error) {
+	termState, err := terminal.GetState(int(syscall.Stdin))
+	if err != nil {
+		return "", err
+	}
+
+	cancel := interrupt.RegisterCleaner(func() error {
+		return terminal.Restore(int(syscall.Stdin), termState)
+	})
+	defer cancel()
+
+	for {
+		fmt.Print("password: ")
+		bytePassword, err := terminal.ReadPassword(int(syscall.Stdin))
+		// new line for coherent formatting, ReadPassword clip the normal new line
+		// entered by the user
+		fmt.Println()
+
+		if err != nil {
+			return "", err
+		}
+
+		if len(bytePassword) > 0 {
+			return string(bytePassword), nil
+		}
+
+		fmt.Println("password is empty")
+	}
+}