Improve error handling for gopass convert (gopasspw#2548)

* Improve error handling for gopass convert Fixes gopasspw#2520 RELEASE_NOTES=[BUGFIX] Improve error handling for gopass convert Signed-off-by: Dominik Schulz <[email protected]> * Ignore cyclop for convert Signed-off-by: Dominik Schulz <[email protected]> --------- Signed-off-by: Dominik Schulz <[email protected]>
sergey-suslov · Feb 9, 2023 · 154d7d4 · 154d7d4
1 parent 4a9fe68
commit 154d7d4
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 14 deletions.
diff --git a/internal/action/convert.go b/internal/action/convert.go
@@ -1,12 +1,12 @@
 package action
 
 import (
-	"fmt"
-
+	"github.com/gopasspw/gopass/internal/action/exit"
 	"github.com/gopasspw/gopass/internal/backend"
 	"github.com/gopasspw/gopass/internal/backend/crypto/age"
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
+	"github.com/gopasspw/gopass/pkg/debug"
 	"github.com/gopasspw/gopass/pkg/termio"
 	"github.com/urfave/cli/v2"
 )
@@ -21,44 +21,48 @@ func (s *Action) Convert(c *cli.Context) error {
 
 	sub, err := s.Store.GetSubStore(store)
 	if err != nil {
-		return fmt.Errorf("mount %q not found: %w", store, err)
+		return exit.Error(exit.NotFound, err, "mount %q not found: %s", store, err)
 	}
 
 	oldStorage := sub.Storage().Name()
 
 	storage, err := backend.StorageRegistry.Backend(oldStorage)
 	if err != nil {
-		return fmt.Errorf("unknown storage backend %q: %w", oldStorage, err)
+		return exit.Error(exit.Unknown, err, "unknown source storage backend %q: %s", oldStorage, err)
 	}
+
 	if sv := c.String("storage"); sv != "" {
 		var err error
 		storage, err = backend.StorageRegistry.Backend(sv)
 		if err != nil {
-			return fmt.Errorf("unknown storage backend %q: %w", sv, err)
+			return exit.Error(exit.Usage, err, "unknown destination storage backend %q: %s", storage, err)
 		}
 	}
 
 	oldCrypto := sub.Crypto().Name()
 
 	crypto, err := backend.CryptoRegistry.Backend(oldCrypto)
 	if err != nil {
-		return fmt.Errorf("unknown crypto backend %q: %w", oldCrypto, err)
+		return exit.Error(exit.Unknown, err, "unknown source crypto backend %q: %s", oldCrypto, err)
 	}
+
 	if sv := c.String("crypto"); sv != "" {
 		var err error
 		crypto, err = backend.CryptoRegistry.Backend(sv)
 		if err != nil {
-			return fmt.Errorf("unknown crypto backend %q: %w", sv, err)
+			return exit.Error(exit.Usage, err, "unknown destination crypto backend %q: %s", sv, err)
 		}
 	}
 
 	if oldCrypto == crypto.String() && oldStorage == storage.String() {
-		out.Notice(ctx, "No conversion needed")
+		out.Notice(ctx, "No conversion needed. Source and destination match.")
 
 		return nil
 	}
 
 	if oldCrypto != crypto.String() {
+		debug.Log("attempting to convert crypto from %q to %q", oldCrypto, crypto.String())
+
 		cbe, err := backend.NewCrypto(ctx, crypto)
 		if err != nil {
 			return err
@@ -82,7 +86,7 @@ func (s *Action) Convert(c *cli.Context) error {
 	}
 
 	if err := s.Store.Convert(ctx, store, crypto, storage, move); err != nil {
-		return fmt.Errorf("failed to convert %q: %w", store, err)
+		return exit.Error(exit.Unknown, err, "failed to convert store %q: %s", store, err)
 	}
 
 	out.OKf(ctx, "Successfully converted %q", store)

diff --git a/internal/store/leaf/convert.go b/internal/store/leaf/convert.go
@@ -22,7 +22,7 @@ import (
 // Convert will convert an existing store to a new store with possibly
 // different set of crypto and storage backends. Please note that it
 // will happily convert to the same set of backends if requested.
-func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, storageBe backend.StorageBackend, move bool) error {
+func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, storageBe backend.StorageBackend, move bool) error { //nolint:cyclop
 	// use a temp queue so we can flush it before removing the old store
 	q := queue.New(ctx)
 	ctx = queue.WithQueue(ctx, q)
@@ -99,11 +99,16 @@ func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, sto
 		}
 		sort.Sort(sort.Reverse(backend.Revisions(revs)))
 
+		// fail if the first revision fails, but if others fail only warn
+		first := true
 		for _, r := range revs {
 			debug.Log("converting %s@%s", e, r.Hash)
 			sec, err := s.GetRevision(ctx, e, r.Hash)
 			if err != nil {
-				return fmt.Errorf("failed to convert revision %s of %s: %w", r.Hash, e, err)
+				if first {
+					return fmt.Errorf("failed to convert revision %s of %s: %w", r.Hash, e, err)
+				}
+				debug.Log("failed to convert revision %s of %s: %w", r.Hash, e, err)
 			}
 
 			msg := fmt.Sprintf("%s\n%s\nCommitted as: %s\nDate: %s\nAuthor: %s <%s>",
@@ -117,8 +122,13 @@ func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, sto
 			ctx := ctxutil.WithCommitMessage(ctx, msg)
 			ctx = ctxutil.WithCommitTimestamp(ctx, r.Date)
 			if err := tmpStore.Set(ctx, e, sec); err != nil {
-				return fmt.Errorf("failed to write converted revision %s of %s to the new store: %w", r.Hash, e, err)
+				if first {
+					return fmt.Errorf("failed to write converted revision %s of %s to the new store: %w", r.Hash, e, err)
+				}
+				debug.Log("failed to write converted revision %s of %s to the new store: %w", r.Hash, e, err)
 			}
+
+			first = false
 		}
 		bar.Inc()
 	}

diff --git a/internal/store/root/convert.go b/internal/store/root/convert.go
@@ -13,13 +13,13 @@ import (
 func (r *Store) Convert(ctx context.Context, name string, cryptoBe backend.CryptoBackend, storageBe backend.StorageBackend, move bool) error {
 	sub, err := r.GetSubStore(name)
 	if err != nil {
-		return fmt.Errorf("mount not found: %w", err)
+		return fmt.Errorf("mount %q not found: %w", name, err)
 	}
 
 	debug.Log("converting %s to crypto: %s, storage: %s", name, cryptoBe, storageBe)
 
 	if err := sub.Convert(ctx, cryptoBe, storageBe, move); err != nil {
-		return fmt.Errorf("failed to convert %q: %w", name, err)
+		return fmt.Errorf("conversion failed: %w", err)
 	}
 
 	if name == "" {