Incorporate review feedback

* Change error handling when we can't find the install time
* Move definition of common runtime functions
* Remove accidental early return

go.sum

@@ -81,6 +81,8 @@ github.com/bugsnag/bugsnag-go v1.5.0 h1:tP8hiPv1pGGW3LA6LKy5lW6WG+y9J2xWUdPd3WC4
 github.com/bugsnag/bugsnag-go v1.5.0/go.mod h1:2oa8nejYd4cQ/b0hMIopN0lCRxU0bueqREvZLWFrtK8=
 github.com/bugsnag/panicwrap v1.2.0 h1:OzrKrRvXis8qEvOkfcxNcYbOd2O7xXS2nnKMEMABFQA=
 github.com/bugsnag/panicwrap v1.2.0/go.mod h1:D/8v3kj0zr8ZAKg1AQ6crr+5VwKN5eIywRkfhyM/+dE=
+github.com/carolynvs/cnab-go v0.12.1-beta1.0.20200611195210-91576308be48 h1:YTtvX0vU5XXLU3ugYfO+tdDmvRPhUO8MsmTjiPx2e+E=
+github.com/carolynvs/cnab-go v0.12.1-beta1.0.20200611195210-91576308be48/go.mod h1:MlIz5HFApIBvFxa5swvb/IxO/DJ4+viUMGb0Dt5tdEg=
 github.com/carolynvs/datetime-printer v0.2.0 h1:Td3FU4YGzx0OogCMhCmLBTUTDPQcq0xlgCeMhAKZmMc=
 github.com/carolynvs/datetime-printer v0.2.0/go.mod h1:p9W8ZUhmQUOVD5kiDuGXwRG65/nTkZWlLylY7s+Qw2k=
 github.com/carolynvs/go-plugin v1.0.1-acceptstdin h1:8JccOWqcZoqCILz191C0D6RTnz/DKfNcY8+T6F3/G9g=

pkg/cnab/provider/action.go

@@ -2,14 +2,17 @@ package cnabprovider
 
 import (
 	"encoding/json"
+	"fmt"
 
 	"get.porter.sh/porter/pkg/config"
+	"github.com/cnabio/cnab-go/action"
+	"github.com/cnabio/cnab-go/claim"
+	"github.com/cnabio/cnab-go/driver"
+	"github.com/cnabio/cnab-go/valuesource"
 	"github.com/cnabio/cnab-to-oci/relocation"
+	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 	"gopkg.in/yaml.v2"
-
-	"github.com/cnabio/cnab-go/action"
-	"github.com/cnabio/cnab-go/driver"
 )
 
 // Shared arguments for all CNAB actions
@@ -102,3 +105,36 @@ func (r *Runtime) AddRelocation(args ActionArguments) action.OperationConfigFunc
 		return nil
 	}
 }
+
+// appendFailedResult creates a failed result from the operation error and accumulates
+// the error(s).
+func (r *Runtime) appendFailedResult(opErr error, c claim.Claim) error {
+	saveResult := func() error {
+		result, err := c.NewResult(claim.StatusFailed)
+		if err != nil {
+			return err
+		}
+		return r.claims.SaveResult(result)
+	}
+
+	resultErr := saveResult()
+
+	// Accumulate any errors from the operation with the persistence errors
+	return multierror.Append(opErr, resultErr).ErrorOrNil()
+}
+
+func (r *Runtime) printDebugInfo(creds valuesource.Set, params map[string]interface{}) {
+	if r.Debug {
+		// only print out the names of the credentials, not the contents, cuz they big and sekret
+		credKeys := make([]string, 0, len(creds))
+		for k := range creds {
+			credKeys = append(credKeys, k)
+		}
+		// param values may also be sensitive, so just print names
+		paramKeys := make([]string, 0, len(params))
+		for k := range params {
+			paramKeys = append(paramKeys, k)
+		}
+		fmt.Fprintf(r.Err, "params: %v\ncreds: %v\n", paramKeys, credKeys)
+	}
+}

pkg/cnab/provider/install.go

@@ -1,12 +1,8 @@
 package cnabprovider
 
 import (
-	"fmt"
-
 	"github.com/cnabio/cnab-go/action"
 	"github.com/cnabio/cnab-go/claim"
-	"github.com/cnabio/cnab-go/valuesource"
-	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 )
 
@@ -54,36 +50,3 @@ func (r *Runtime) Install(args ActionArguments) error {
 
 	return a.SaveOperationResult(opResult, c, result)
 }
-
-// appendFailedResult creates a failed result from the operation error and accumulates
-// the error(s).
-func (r *Runtime) appendFailedResult(opErr error, c claim.Claim) error {
-	saveResult := func() error {
-		result, err := c.NewResult(claim.StatusFailed)
-		if err != nil {
-			return err
-		}
-		return r.claims.SaveResult(result)
-	}
-
-	resultErr := saveResult()
-
-	// Accumulate any errors from the operation with the persistence errors
-	return multierror.Append(opErr, resultErr).ErrorOrNil()
-}
-
-func (r *Runtime) printDebugInfo(creds valuesource.Set, params map[string]interface{}) {
-	if r.Debug {
-		// only print out the names of the credentials, not the contents, cuz they big and sekret
-		credKeys := make([]string, 0, len(creds))
-		for k := range creds {
-			credKeys = append(credKeys, k)
-		}
-		// param values may also be sensitive, so just print names
-		paramKeys := make([]string, 0, len(params))
-		for k := range params {
-			paramKeys = append(paramKeys, k)
-		}
-		fmt.Fprintf(r.Err, "params: %v\ncreds: %v\n", paramKeys, credKeys)
-	}
-}

pkg/cnab/provider/runtime.go

@@ -4,10 +4,10 @@ import (
 	"get.porter.sh/porter/pkg/cnab/extensions"
 	"get.porter.sh/porter/pkg/config"
 	"get.porter.sh/porter/pkg/credentials"
+	"get.porter.sh/porter/pkg/parameters"
 	"github.com/cnabio/cnab-go/bundle"
 	"github.com/cnabio/cnab-go/claim"
 	"github.com/pkg/errors"
-	"get.porter.sh/porter/pkg/parameters"
 )
 
 var _ CNABProvider = &Runtime{}

pkg/porter/list.go

@@ -29,14 +29,17 @@ type DisplayInstallation struct {
 }
 
 func NewDisplayInstallation(installation claim.Installation) (DisplayInstallation, error) {
-	installTime, err := installation.GetInstallationTimestamp()
+	c, err := installation.GetLastClaim()
 	if err != nil {
 		return DisplayInstallation{}, err
 	}
 
-	c, err := installation.GetLastClaim()
+	installTime, err := installation.GetInstallationTimestamp()
 	if err != nil {
-		return DisplayInstallation{}, err
+		// if we cannot determine when the bundle was installed,
+		// for example it hasn't had install run yet, only an action like dry-run
+		// just use the timestamp from the claim
+		installTime = c.Created
 	}
 
 	return DisplayInstallation{
@@ -73,8 +76,7 @@ func (p *Porter) ListInstances(opts ListOptions) error {
 	for _, installation := range installations {
 		displayInstallation, err := NewDisplayInstallation(installation)
 		if err != nil {
-			// There isn't an installation for this result
-			continue
+			return err
 		}
 		displayInstallations = append(displayInstallations, displayInstallation)
 	}

pkg/porter/list_test.go

@@ -0,0 +1,73 @@
+package porter
+
+import (
+	"testing"
+
+	"github.com/cnabio/cnab-go/bundle"
+	"github.com/cnabio/cnab-go/claim"
+	"github.com/cnabio/cnab-go/utils/crud"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewDisplayInstallation(t *testing.T) {
+	bun := bundle.Bundle{Name: "wordpress"}
+
+	t.Run("install exists", func(t *testing.T) {
+		install, err := claim.New("wordpress", claim.ActionInstall, bun, nil)
+		require.NoError(t, err, "New claim failed")
+		installResult, err := install.NewResult(claim.StatusSucceeded)
+		require.NoError(t, err, "NewResult failed")
+
+		upgrade, err := claim.New("wordpress", claim.ActionUpgrade, bun, nil)
+		require.NoError(t, err, "New claim failed")
+		upgradeResult, err := upgrade.NewResult(claim.StatusRunning)
+		require.NoError(t, err, "NewResult failed")
+
+		cp := claim.NewClaimStore(crud.NewMockStore(), nil, nil)
+		err = cp.SaveClaim(install)
+		require.NoError(t, err, "SaveClaim failed")
+		err = cp.SaveResult(installResult)
+		require.NoError(t, err, "SaveResult failed")
+		err = cp.SaveClaim(upgrade)
+		require.NoError(t, err, "SaveClaim failed")
+		err = cp.SaveResult(upgradeResult)
+		require.NoError(t, err, "SaveResult failed")
+
+		i, err := cp.ReadInstallation("wordpress")
+		require.NoError(t, err, "ReadInstallation failed")
+
+		di, err := NewDisplayInstallation(i)
+		require.NoError(t, err, "NewDisplayInstallation failed")
+
+		require.Equal(t, di.Name, i.Name, "invalid installation name")
+		require.True(t, install.Created.Equal(di.Created), "invalid created time")
+		require.True(t, upgrade.Created.Equal(di.Modified), "invalid modified time")
+		require.Equal(t, claim.ActionUpgrade, di.Action, "invalid last action")
+		require.Equal(t, claim.StatusRunning, di.Status, "invalid last status")
+	})
+
+	t.Run("install does not exist", func(t *testing.T) {
+		dryRun, err := claim.New("wordpress", "dry-run", bun, nil)
+		require.NoError(t, err, "New claim failed")
+		dryRunResult, err := dryRun.NewResult(claim.StatusSucceeded)
+		require.NoError(t, err, "NewResult failed")
+
+		cp := claim.NewClaimStore(crud.NewMockStore(), nil, nil)
+		err = cp.SaveClaim(dryRun)
+		require.NoError(t, err, "SaveClaim failed")
+		err = cp.SaveResult(dryRunResult)
+		require.NoError(t, err, "SaveResult failed")
+
+		i, err := cp.ReadInstallation("wordpress")
+		require.NoError(t, err, "ReadInstallation failed")
+
+		di, err := NewDisplayInstallation(i)
+		require.NoError(t, err, "NewDisplayInstallation failed")
+
+		require.Equal(t, di.Name, i.Name, "invalid installation name")
+		require.True(t, dryRun.Created.Equal(di.Created), "invalid created time")
+		require.True(t, dryRun.Created.Equal(di.Modified), "invalid modified time")
+		require.Equal(t, "dry-run", di.Action, "invalid last action")
+		require.Equal(t, claim.StatusSucceeded, di.Status, "invalid last status")
+	})
+}

pkg/storage/pluginstore/store.go

@@ -67,7 +67,6 @@ func (s *Store) Connect() error {
 }
 
 func (s *Store) Close() error {
-	return nil
 	if s.cleanup != nil {
 		s.cleanup()
 	}