Clean up LockInfo and LockError and use them

Gove LockInfo a Marshal method for easy serialization, and a String
method for more readable output.

Have the state.Locker implementations use LockError when possible to
return LockInfo and an error.

backend/remote-state/consul/client.go

@@ -4,10 +4,10 @@ import (
 	"crypto/md5"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"time"
 
 	consulapi "github.com/hashicorp/consul/api"
-	"github.com/hashicorp/errwrap"
 	multierror "github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/state/remote"
@@ -62,15 +62,10 @@ func (c *RemoteClient) putLockInfo(info *state.LockInfo) error {
 	info.Path = c.Path
 	info.Created = time.Now().UTC()
 
-	js, err := json.Marshal(info)
-	if err != nil {
-		return err
-	}
-
 	kv := c.Client.KV()
-	_, err = kv.Put(&consulapi.KVPair{
+	_, err := kv.Put(&consulapi.KVPair{
 		Key:   c.Path + lockInfoSuffix,
-		Value: js,
+		Value: info.Marshal(),
 	}, nil)
 
 	return err
@@ -89,7 +84,7 @@ func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) {
 	li := &state.LockInfo{}
 	err = json.Unmarshal(pair.Value, li)
 	if err != nil {
-		return nil, errwrap.Wrapf("error unmarshaling lock info: {{err}}", err)
+		return nil, fmt.Errorf("error unmarshaling lock info: %s", err)
 	}
 
 	return li, nil
@@ -126,24 +121,33 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) {
 		c.consulLock = lock
 	}
 
+	lockErr := &state.LockError{}
+
 	lockCh, err := c.consulLock.Lock(make(chan struct{}))
 	if err != nil {
-		return "", err
+		lockErr.Err = err
+		return "", lockErr
 	}
 
 	if lockCh == nil {
 		lockInfo, e := c.getLockInfo()
 		if e != nil {
-			return "", e
+			lockErr.Err = e
+			return "", lockErr
 		}
-		return "", lockInfo.Err()
+
+		lockErr.Info = lockInfo
+		return "", lockErr
 	}
 
 	c.lockCh = lockCh
 
 	err = c.putLockInfo(info)
 	if err != nil {
-		err = multierror.Append(err, c.Unlock(info.ID))
+		if unlockErr := c.Unlock(info.ID); unlockErr != nil {
+			err = multierror.Append(err, unlockErr)
+		}
+
 		return "", err
 	}
 

command/apply_destroy_test.go

@@ -140,7 +140,7 @@ func TestApply_destroyLockedState(t *testing.T) {
 	}
 
 	output := ui.ErrorWriter.String()
-	if !strings.Contains(output, "locked") {
+	if !strings.Contains(output, "lock") {
 		t.Fatal("command output does not look like a lock error:", output)
 	}
 }

command/apply_test.go

@@ -87,7 +87,7 @@ func TestApply_lockedState(t *testing.T) {
 	}
 
 	output := ui.ErrorWriter.String()
-	if !strings.Contains(output, "locked") {
+	if !strings.Contains(output, "lock") {
 		t.Fatal("command output does not look like a lock error:", output)
 	}
 }

command/plan_test.go

@@ -72,7 +72,7 @@ func TestPlan_lockedState(t *testing.T) {
 	}
 
 	output := ui.ErrorWriter.String()
-	if !strings.Contains(output, "locked") {
+	if !strings.Contains(output, "lock") {
 		t.Fatal("command output does not look like a lock error:", output)
 	}
 }

command/refresh_test.go

@@ -91,7 +91,7 @@ func TestRefresh_lockedState(t *testing.T) {
 	}
 
 	output := ui.ErrorWriter.String()
-	if !strings.Contains(output, "locked") {
+	if !strings.Contains(output, "lock") {
 		t.Fatal("command output does not look like a lock error:", output)
 	}
 }

command/taint_test.go

@@ -84,7 +84,7 @@ func TestTaint_lockedState(t *testing.T) {
 	}
 
 	output := ui.ErrorWriter.String()
-	if !strings.Contains(output, "locked") {
+	if !strings.Contains(output, "lock") {
 		t.Fatal("command output does not look like a lock error:", output)
 	}
 }

command/untaint_test.go

@@ -90,7 +90,7 @@ func TestUntaint_lockedState(t *testing.T) {
 	}
 
 	output := ui.ErrorWriter.String()
-	if !strings.Contains(output, "locked") {
+	if !strings.Contains(output, "lock") {
 		t.Fatal("command output does not look like a lock error:", output)
 	}
 }

state/local.go

@@ -10,7 +10,7 @@ import (
 	"path/filepath"
 	"time"
 
-	"github.com/hashicorp/errwrap"
+	multierror "github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform/terraform"
 )
 
@@ -148,11 +148,17 @@ func (s *LocalState) Lock(info *LockInfo) (string, error) {
 	}
 
 	if err := s.lock(); err != nil {
-		if info, err := s.lockInfo(); err == nil {
+		info, infoErr := s.lockInfo()
-			return "", info.Err()
+		if infoErr != nil {
+			err = multierror.Append(err, infoErr)
 		}
 
-		return "", fmt.Errorf("state file %q locked: %s", s.Path, err)
+		lockErr := &LockError{
+			Info: info,
+			Err:  err,
+		}
+
+		return "", lockErr
 	}
 
 	s.lockID = info.ID
@@ -167,8 +173,8 @@ func (s *LocalState) Unlock(id string) error {
 	if id != s.lockID {
 		idErr := fmt.Errorf("invalid lock id: %q. current id: %q", id, s.lockID)
 		info, err := s.lockInfo()
-		if err == nil {
+		if err != nil {
-			return errwrap.Wrap(idErr, err)
+			err = multierror.Append(idErr, err)
 		}
 
 		return &LockError{
@@ -257,12 +263,7 @@ func (s *LocalState) writeLockInfo(info *LockInfo) error {
 	info.Path = s.Path
 	info.Created = time.Now().UTC()
 
-	infoData, err := json.Marshal(info)
+	err := ioutil.WriteFile(path, info.Marshal(), 0600)
-	if err != nil {
-		panic(fmt.Sprintf("could not marshal lock info: %#v", info))
-	}
-
-	err = ioutil.WriteFile(path, infoData, 0600)
 	if err != nil {
 		return fmt.Errorf("could not write lock info for %q: %s", s.Path, err)
 	}

state/remote/s3.go

@@ -223,7 +223,7 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) {
 	putParams := &dynamodb.PutItemInput{
 		Item: map[string]*dynamodb.AttributeValue{
 			"LockID": {S: aws.String(stateName)},
-			"Info":   {S: aws.String(info.String())},
+			"Info":   {S: aws.String(string(info.Marshal()))},
 		},
 		TableName:           aws.String(c.lockTable),
 		ConditionExpression: aws.String("attribute_not_exists(LockID)"),
@@ -231,12 +231,16 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) {
 	_, err := c.dynClient.PutItem(putParams)
 
 	if err != nil {
-		lockInfo, err := c.getLockInfo()
+		lockInfo, infoErr := c.getLockInfo()
-		if err != nil {
+		if infoErr != nil {
-			return "", fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err)
+			err = multierror.Append(err, infoErr)
 		}
 
-		return info.ID, lockInfo.Err()
+		lockErr := &state.LockError{
+			Err:  err,
+			Info: lockInfo,
+		}
+		return "", lockErr
 	}
 	return info.ID, nil
 }
@@ -274,16 +278,21 @@ func (c *S3Client) Unlock(id string) error {
 		return nil
 	}
 
+	lockErr := &state.LockError{}
+
 	// TODO: store the path and lock ID in separate fields, and have proper
 	// projection expression only delete the lock if both match, rather than
 	// checking the ID from the info field first.
 	lockInfo, err := c.getLockInfo()
 	if err != nil {
-		return fmt.Errorf("failed to retrieve lock info: %s", err)
+		lockErr.Err = fmt.Errorf("failed to retrieve lock info: %s", err)
+		return lockErr
 	}
+	lockErr.Info = lockInfo
 
 	if lockInfo.ID != id {
-		return fmt.Errorf("lock id %q does not match existing lock", id)
+		lockErr.Err = fmt.Errorf("lock id %q does not match existing lock", id)
+		return lockErr
 	}
 
 	params := &dynamodb.DeleteItemInput{
@@ -295,7 +304,8 @@ func (c *S3Client) Unlock(id string) error {
 	_, err = c.dynClient.DeleteItem(params)
 
 	if err != nil {
-		return err
+		lockErr.Err = err
+		return lockErr
 	}
 	return nil
 }

state/state.go

@@ -1,12 +1,15 @@
 package state
 
 import (
+	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"math/rand"
 	"os"
 	"os/user"
 	"strings"
+	"text/template"
 	"time"
 
 	uuid "github.com/hashicorp/go-uuid"
@@ -84,12 +87,15 @@ func NewLockInfo() *LockInfo {
 	}
 
 	// don't error out on user and hostname, as we don't require them
-	username, _ := user.Current()
+	userName := ""
+	if userInfo, err := user.Current(); err == nil {
+		userName = userInfo.Username
+	}
 	host, _ := os.Hostname()
 
 	info := &LockInfo{
 		ID:      id,
-		Who:     fmt.Sprintf("%s@%s", username, host),
+		Who:     fmt.Sprintf("%s@%s", userName, host),
 		Version: terraform.Version,
 		Created: time.Now().UTC(),
 	}
@@ -123,16 +129,36 @@ type LockInfo struct {
 
 // Err returns the lock info formatted in an error
 func (l *LockInfo) Err() error {
-	return fmt.Errorf("state locked. path:%q, created:%s, info:%q",
+	return errors.New(l.String())
-		l.Path, l.Created, l.Info)
 }
 
-func (l *LockInfo) String() string {
+// Marshal returns a string json representation of the LockInfo
+func (l *LockInfo) Marshal() []byte {
 	js, err := json.Marshal(l)
 	if err != nil {
 		panic(err)
 	}
-	return string(js)
+	return js
+}
+
+// String return a multi-line string representation of LockInfo
+func (l *LockInfo) String() string {
+	tmpl := `Lock Info:
+  ID:        {{.ID}}
+  Path:      {{.Path}}
+  Operation: {{.Operation}}
+  Who:       {{.Who}}
+  Version:   {{.Version}}
+  Created:   {{.Created}}
+  Info:      {{.Info}}
+`
+
+	t := template.Must(template.New("LockInfo").Parse(tmpl))
+	var out bytes.Buffer
+	if err := t.Execute(&out, l); err != nil {
+		panic(err)
+	}
+	return out.String()
 }
 
 type LockError struct {
@@ -147,7 +173,7 @@ func (e *LockError) Error() string {
 	}
 
 	if e.Info != nil {
-		out = append(out, e.Info.Err().Error())
+		out = append(out, e.Info.String())
 	}
 	return strings.Join(out, "\n")
 }

state/state_test.go

@@ -1,6 +1,7 @@
 package state
 
 import (
+	"encoding/json"
 	"flag"
 	"io/ioutil"
 	"log"
@@ -41,4 +42,11 @@ func TestNewLockInfo(t *testing.T) {
 	if info1.ID == info2.ID {
 		t.Fatal("multiple LockInfo with identical IDs")
 	}
+
+	// test the JSON output is valid
+	newInfo := &LockInfo{}
+	err := json.Unmarshal(info1.Marshal(), newInfo)
+	if err != nil {
+		t.Fatal(err)
+	}
 }