cli: improved 'snapshot delete' usage (#436)

New usage:

```
kopia snapshot delete manifestID... [--delete]
kopia snapshot delete rootObjectID... [--delete]
```

Fixes #435

cli: added --unsafe-ignore-source as alias for `--delete`
This is a hidden flag for backwards compatibility. It will be removed.

cli/command_snapshot_delete.go

@@ -2,66 +2,90 @@
 
 import (
 	"context"
+	"fmt"
+	"strings"
 
 	"github.com/pkg/errors"
 
 	"github.com/kopia/kopia/repo"
 	"github.com/kopia/kopia/repo/manifest"
+	"github.com/kopia/kopia/repo/object"
+	"github.com/kopia/kopia/snapshot"
 )
 
 var (
-	snapshotDeleteCommand      = snapshotCommands.Command("delete", "Explicitly delete a snapshot by providing a snapshot ID.")
-	snapshotDeleteID           = snapshotDeleteCommand.Arg("id", "Snapshot ID to be deleted").Required().String()
-	snapshotDeletePath         = snapshotDeleteCommand.Flag("path", "Specify the path of the snapshot to be deleted").String()
-	snapshotDeleteHostname     = snapshotDeleteCommand.Flag("hostname", "Specify the hostname of the snapshot to be deleted").String()
-	snapshotDeleteUsername     = snapshotDeleteCommand.Flag("username", "Specify the username of the snapshot to be deleted").String()
-	snapshotDeleteIgnoreSource = snapshotDeleteCommand.Flag("unsafe-ignore-source", "Override the requirement to specify source info for the delete to succeed").Bool()
+	snapshotDeleteCommand = snapshotCommands.Command("delete", "Explicitly delete a snapshot by providing a snapshot ID.")
+	snapshotDeleteIDs     = snapshotDeleteCommand.Arg("id", "Snapshot ID or root object ID to be deleted").Required().Strings()
+	snapshotDeleteConfirm = snapshotDeleteCommand.Flag("delete", "Confirm deletion").Bool()
 )
 
 func runDeleteCommand(ctx context.Context, rep repo.Repository) error {
-	if !*snapshotDeleteIgnoreSource && *snapshotDeletePath == "" {
-		return errors.New("path is required")
+	for _, id := range *snapshotDeleteIDs {
+		if strings.HasPrefix(id, "k") {
+			if err := deleteSnapshotsByRootObjectID(ctx, rep, object.ID(id)); err != nil {
+				return errors.Wrapf(err, "error deleting snapshots by root ID %v", id)
+			}
+		} else {
+			m, err := snapshot.LoadSnapshot(ctx, rep, manifest.ID(id))
+			if err != nil {
+				return errors.Wrapf(err, "error loading snapshot %v", id)
+			}
+
+			if err := deleteSnapshot(ctx, rep, m); err != nil {
+				return errors.Wrapf(err, "error deleting %v", id)
+			}
+		}
 	}
 
-	manifestID := manifest.ID(*snapshotDeleteID)
+	return nil
+}
 
-	manifestMeta, err := rep.GetManifest(ctx, manifestID, nil)
+func deleteSnapshot(ctx context.Context, rep repo.Repository, m *snapshot.Manifest) error {
+	desc := fmt.Sprintf("snapshot %v of %v at %v", m.ID, m.Source, formatTimestamp(m.StartTime))
+
+	if !*snapshotDeleteConfirm {
+		printStderr("Would delete %v (pass --delete to confirm)\n", desc)
+		return nil
+	}
+
+	printStderr("Deleting %v...\n", desc)
+
+	return rep.DeleteManifest(ctx, m.ID)
+}
+
+func deleteSnapshotsByRootObjectID(ctx context.Context, rep repo.Repository, rootID object.ID) error {
+	ids, err := snapshot.ListSnapshotManifests(ctx, rep, nil)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "error listing snapshot manifests")
 	}
 
-	labels := manifestMeta.Labels
-	if labels["type"] != "snapshot" {
-		return errors.Errorf("snapshot ID provided (%v) did not reference a snapshot", manifestID)
+	manifests, err := snapshot.LoadSnapshots(ctx, rep, ids)
+	if err != nil {
+		return errors.Wrap(err, "error loading snapshot manifests")
 	}
 
-	if !*snapshotDeleteIgnoreSource {
-		h := *snapshotDeleteHostname
-		if h == "" {
-			h = rep.Hostname()
-		}
+	cnt := 0
 
-		if labels["hostname"] != h {
-			return errors.Errorf("host name does not match for deleting requested snapshot ID (got %q, expected %q)", h, labels["hostname"])
-		}
+	for _, m := range manifests {
+		if m.RootObjectID() == rootID {
+			cnt++
 
-		u := *snapshotDeleteUsername
-		if u == "" {
-			u = rep.Username()
-		}
-
-		if labels["username"] != u {
-			return errors.Errorf("user name does not match for deleting requested snapshot ID (got %q, expected %q)", u, labels["username"])
-		}
-
-		if labels["path"] != *snapshotDeletePath {
-			return errors.New("path does not match for deleting requested snapshot ID")
+			if err := deleteSnapshot(ctx, rep, m); err != nil {
+				return errors.Wrap(err, "error deleting")
+			}
 		}
 	}
 
-	return rep.DeleteManifest(ctx, manifestID)
+	if cnt == 0 {
+		return errors.Errorf("no snapshots matched %v", rootID)
+	}
+
+	return nil
 }
 
 func init() {
 	snapshotDeleteCommand.Action(repositoryAction(runDeleteCommand))
+
+	// hidden flag for backwards compatibility
+	snapshotDeleteCommand.Flag("unsafe-ignore-source", "Alias for --delete").Hidden().BoolVar(snapshotDeleteConfirm)
 }

snapshot/manager.go

@@ -70,10 +70,16 @@ func ListSnapshots(ctx context.Context, rep repo.Repository, si SourceInfo) ([]*
 // LoadSnapshot loads and parses a snapshot with a given ID.
 func LoadSnapshot(ctx context.Context, rep repo.Repository, manifestID manifest.ID) (*Manifest, error) {
 	sm := &Manifest{}
-	if _, err := rep.GetManifest(ctx, manifestID, sm); err != nil {
+
+	em, err := rep.GetManifest(ctx, manifestID, sm)
+	if err != nil {
 		return nil, errors.Wrap(err, "unable to find manifest entries")
 	}
 
+	if em.Labels[manifest.TypeLabelKey] != ManifestType {
+		return nil, errors.Errorf("manifest is not a snapshot")
+	}
+
 	sm.ID = manifestID
 
 	return sm, nil

tests/end_to_end_test/snapshot_delete_test.go

@@ -10,7 +10,7 @@
 	"github.com/kopia/kopia/tests/testenv"
 )
 
-type deleteArgMaker func(manifestID string, source testenv.SourceInfo) []string
+type deleteArgMaker func(manifestID, objectID string, source testenv.SourceInfo) []string
 
 //nolint:funlen
 func TestSnapshotDelete(t *testing.T) {
@@ -28,181 +28,73 @@ func TestSnapshotDelete(t *testing.T) {
 	}{
 		{
 			"Test manifest rm function",
-			func(manifestID string, source testenv.SourceInfo) []string {
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
 				return []string{"manifest", "rm", manifestID}
 			},
 			expectSuccess,
 		},
 		{
-			"Specify all source values correctly",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--username", source.User,
-					"--path", source.Path,
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"Specify path and username, using default hostname",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--username", source.User,
-					"--path", source.Path,
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"Specify path and hostname, using default username",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--path", source.Path,
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"No source flags, with unsafe ignore source flag",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--unsafe-ignore-source",
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"Specify path only, using default username and hostname",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--path", source.Path,
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"Specify all source flags, incorrect host name",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", "some-other-host",
-					"--username", source.User,
-					"--path", source.Path,
-				}
-			},
-			expectFail,
-		},
-		{
-			"Specify all source flags, incorrect user name",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--username", "some-other-user",
-					"--path", source.Path,
-				}
-			},
-			expectFail,
-		},
-		{
-			"Specify all source flags, incorrect path",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--username", source.User,
-					"--path", "some-wrong-path",
-				}
-			},
-			expectFail,
-		},
-		{
-			"Specify all source flags, incorrect hostname, ignore flag set",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--unsafe-ignore-source",
-					"--hostname", "some-other-host",
-					"--username", source.User,
-					"--path", source.Path,
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"Specify all source flags, incorrect username, ignore flag set",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--username", "some-other-user",
-					"--unsafe-ignore-source",
-					"--path", source.Path,
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"Specify all source flags, incorrect path, ignore flag set",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--username", source.User,
-					"--path", "some-wrong-path",
-					"--unsafe-ignore-source",
-				}
-			},
-			expectSuccess,
-		},
-		{
-			"No manifest ID provided",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete"}
-			},
-			expectFail,
-		},
-		{
-			"No manifest ID provided, ignore source flag set",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete",
-					"--unsafe-ignore-source",
-				}
-			},
-			expectFail,
-		},
-		{
-			"Garbage manifest ID provided",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", "some-garbage-manifestID"}
-			},
-			expectFail,
-		},
-		{
-			"Hostname flag provided but no value input",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname",
-					"--username", source.User,
-					"--path", source.Path,
-				}
-			},
-			expectFail,
-		},
-		{
-			"No path provided and no unsafe ignore source flag provided",
-			func(manifestID string, source testenv.SourceInfo) []string {
+			"Dry run - by manifest ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
 				return []string{"snapshot", "delete", manifestID}
 			},
-			expectFail,
+			expectSuccess,
 		},
 		{
-			"Specify hostname and username with no path provided",
-			func(manifestID string, source testenv.SourceInfo) []string {
-				return []string{"snapshot", "delete", manifestID,
-					"--hostname", source.Host,
-					"--username", source.User,
-				}
+			"Delete - by manifest ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", manifestID, "--delete"}
+			},
+			expectSuccess,
+		},
+		{
+			"Delete - by manifest ID - legacy flag",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", manifestID, "--unsafe-ignore-source"}
+			},
+			expectSuccess,
+		},
+		{
+			"Dry run - by objectID ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", objectID}
+			},
+			expectSuccess,
+		},
+		{
+			"Delete - by object ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", objectID, "--delete"}
+			},
+			expectSuccess,
+		}, {
+			"Dry run - invalid object ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", "no-such-manifest"}
 			},
 			expectFail,
 		},
-	} {
+		{
+			"Delete - invalid manifest ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", "no-such-manifest", "--delete"}
+			},
+			expectFail,
+		},
+		{
+			"Dry run - invalid object ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", "k001122"}
+			},
+			expectFail,
+		},
+		{
+			"Delete - invalid object ID",
+			func(manifestID, objectID string, source testenv.SourceInfo) []string {
+				return []string{"snapshot", "delete", "k001122", "--delete"}
+			},
+			expectFail,
+		}} {
 		t.Log(tc.desc)
 		testSnapshotDelete(t, tc.mf, tc.expectSuccess)
 	}
@@ -230,8 +122,7 @@ func testSnapshotDelete(t *testing.T, argMaker deleteArgMaker, expectDeleteSucce
 	for _, source := range si {
 		for _, ss := range source.Snapshots {
 			manifestID := ss.SnapshotID
-			args := argMaker(manifestID, source)
-			t.Logf("manifestID: %v", manifestID)
+			args := argMaker(manifestID, ss.ObjectID, source)
 
 			if expectDeleteSucceeds {
 				e.RunAndExpectSuccess(t, args...)
@@ -270,7 +161,7 @@ func TestSnapshotDeleteTypeCheck(t *testing.T) {
 			t.Fatalf("Expected global policy manifest on a fresh repo")
 		}
 
-		e.RunAndExpectFailure(t, "snapshot", "delete", manifestID, "--unsafe-ignore-source")
+		e.RunAndExpectFailure(t, "snapshot", "delete", manifestID)
 	}
 }
 
@@ -317,10 +208,10 @@ func TestSnapshotDeleteRestore(t *testing.T) {
 	compareDirs(t, source, restoreDir)
 
 	// snapshot delete should succeed
-	e.RunAndExpectSuccess(t, "snapshot", "delete", snapID, "--unsafe-ignore-source")
+	e.RunAndExpectSuccess(t, "snapshot", "delete", snapID, "--delete")
 
 	// Subsequent snapshot delete to the same ID should fail
-	e.RunAndExpectFailure(t, "snapshot", "delete", snapID, "--unsafe-ignore-source")
+	e.RunAndExpectFailure(t, "snapshot", "delete", snapID, "--delete")
 
 	// garbage-collect to clean up the root object. Otherwise
 	// a restore will succeed

tests/tools/kopiarunner/kopia_snapshotter.go

@@ -100,7 +100,7 @@ func (ks *KopiaSnapshotter) RestoreSnapshot(snapID, restoreDir string) (err erro
 // DeleteSnapshot implements the Snapshotter interface, issues a kopia snapshot
 // delete of the provided snapshot ID
 func (ks *KopiaSnapshotter) DeleteSnapshot(snapID string) (err error) {
-	_, _, err = ks.Runner.Run("snapshot", "delete", snapID, "--unsafe-ignore-source")
+	_, _, err = ks.Runner.Run("snapshot", "delete", snapID, "--delete")
 	return err
 }