fix(cli): preserve error exit code when '--json' output is specified (#3163)

Changes kopia's behavior to match the exit code that would
have been returned when the `--json` flag was not specified.

`kopia snapshot create my/path --json` terminates with a 0
status code in cases where
`kopia snapshot create my/path` terminates with a
 non-zero exit code.

One such case is when there are permissions errors reading
files or directories to snapshot.

Adds end-to-end tests for snapshot create with '--json' flag

cli/command_snapshot_create.go

@@ -349,11 +349,10 @@ func (c *commandSnapshotCreate) reportSnapshotStatus(ctx context.Context, manife
 
 	if c.jo.jsonOutput {
 		c.out.printStdout("%s\n", c.jo.jsonIndentedBytes(manifest, "  "))
-		return nil
+	} else {
+		log(ctx).Infof("Created%v snapshot with root %v and ID %v in %v", maybePartial, manifest.RootObjectID(), snapID, manifest.EndTime.Sub(manifest.StartTime).Truncate(time.Second))
 	}
 
-	log(ctx).Infof("Created%v snapshot with root %v and ID %v in %v", maybePartial, manifest.RootObjectID(), snapID, manifest.EndTime.Sub(manifest.StartTime).Truncate(time.Second))
-
 	if ds := manifest.RootEntry.DirSummary; ds != nil {
 		if ds.IgnoredErrorCount > 0 {
 			log(ctx).Warnf("Ignored %v error(s) while snapshotting %v.", ds.IgnoredErrorCount, sourceInfo)

tests/end_to_end_test/restore_fail_test.go

@@ -51,8 +51,8 @@ func TestRestoreFail(t *testing.T) {
 
 	beforeBlobList := e.RunAndExpectSuccess(t, "blob", "list")
 
-	_, errOut := e.RunAndExpectSuccessWithErrOut(t, "snapshot", "create", sourceDir)
-	parsed := parseSnapshotResult(t, errOut)
+	out, errOut := e.RunAndExpectSuccessWithErrOut(t, "snapshot", "create", sourceDir)
+	parsed := parseSnapshotResultFromLog(t, out, errOut)
 
 	afterBlobList := e.RunAndExpectSuccess(t, "blob", "list")
 

tests/end_to_end_test/snapshot_fail_test.go

@@ -14,6 +14,7 @@
 	"github.com/stretchr/testify/require"
 
 	"github.com/kopia/kopia/internal/testutil"
+	"github.com/kopia/kopia/snapshot"
 	"github.com/kopia/kopia/tests/testdirtree"
 	"github.com/kopia/kopia/tests/testenv"
 )
@@ -36,22 +37,27 @@ func TestSnapshotNonexistent(t *testing.T) {
 
 func TestSnapshotFail_Default(t *testing.T) {
 	t.Parallel()
-	testSnapshotFail(t, false, nil, nil)
+	testSnapshotFailText(t, false, nil, nil)
+}
+
+func TestSnapshotFail_DefaultJSONOutput(t *testing.T) {
+	t.Parallel()
+	testSnapshotFail(t, false, []string{"--json"}, nil, parseSnapshotResultJSON)
 }
 
 func TestSnapshotFail_EnvOverride(t *testing.T) {
 	t.Parallel()
-	testSnapshotFail(t, true, nil, map[string]string{"KOPIA_SNAPSHOT_FAIL_FAST": "true"})
+	testSnapshotFailText(t, true, nil, map[string]string{"KOPIA_SNAPSHOT_FAIL_FAST": "true"})
 }
 
 func TestSnapshotFail_NoFailFast(t *testing.T) {
 	t.Parallel()
-	testSnapshotFail(t, false, []string{"--no-fail-fast"}, nil)
+	testSnapshotFailText(t, false, []string{"--no-fail-fast"}, nil)
 }
 
 func TestSnapshotFail_FailFast(t *testing.T) {
 	t.Parallel()
-	testSnapshotFail(t, true, []string{"--fail-fast"}, nil)
+	testSnapshotFailText(t, true, []string{"--fail-fast"}, nil)
 }
 
 type expectedSnapshotResult struct {
@@ -69,8 +75,20 @@ func cond(c bool, a, b int) int {
 	return b
 }
 
+func testSnapshotFailText(t *testing.T, isFailFast bool, snapshotCreateFlags []string, snapshotCreateEnv map[string]string) {
+	t.Helper()
+
+	testSnapshotFail(t, isFailFast, snapshotCreateFlags, snapshotCreateEnv, parseSnapshotResultFromLog)
+}
+
 //nolint:thelper,cyclop
-func testSnapshotFail(t *testing.T, isFailFast bool, snapshotCreateFlags []string, snapshotCreateEnv map[string]string) {
+func testSnapshotFail(
+	t *testing.T,
+	isFailFast bool,
+	snapshotCreateFlags []string,
+	snapshotCreateEnv map[string]string,
+	parseSnapshotResultFn func(t *testing.T, stdOut, _ []string) parsedSnapshotResult,
+) {
 	if runtime.GOOS == windowsOSName {
 		t.Skip("this test does not work on Windows")
 	}
@@ -79,7 +97,7 @@ func testSnapshotFail(t *testing.T, isFailFast bool, snapshotCreateFlags []strin
 		t.Skip("this test does not work as root, because we're unable to remove permissions.")
 	}
 
-	dir0Path := "dir0"
+	const dir0Path = "dir0"
 
 	for _, ignoreFileErr := range []string{"true", "false"} {
 		for _, ignoreDirErr := range []string{"true", "false"} {
@@ -260,7 +278,7 @@ func testSnapshotFail(t *testing.T, isFailFast bool, snapshotCreateFlags []strin
 
 					e.RunAndExpectSuccess(t, "policy", "set", snapSource, "--ignore-dir-errors", tcIgnoreDirErr, "--ignore-file-errors", tcIgnoreFileErr)
 					restoreDir := fmt.Sprintf("%s%d_%v_%v", restoreDirPrefix, tcIdx, tcIgnoreDirErr, tcIgnoreFileErr)
-					testPermissions(t, e, snapSource, modifyEntry, restoreDir, tc.expectSuccess, snapshotCreateFlags, snapshotCreateEnv)
+					testPermissions(t, e, snapSource, modifyEntry, restoreDir, tc.expectSuccess, snapshotCreateFlags, snapshotCreateEnv, parseSnapshotResultFn)
 
 					e.RunAndExpectSuccess(t, "policy", "remove", snapSource)
 				})
@@ -300,7 +318,15 @@ func createSimplestFileTree(t *testing.T, dirDepth, currDepth int, currPath stri
 // It returns the number of successful snapshot operations.
 //
 //nolint:thelper
-func testPermissions(t *testing.T, e *testenv.CLITest, source, modifyEntry, restoreDir string, expect map[os.FileMode]expectedSnapshotResult, snapshotCreateFlags []string, snapshotCreateEnv map[string]string) int {
+func testPermissions(
+	t *testing.T,
+	e *testenv.CLITest,
+	source, modifyEntry, restoreDir string,
+	expect map[os.FileMode]expectedSnapshotResult,
+	snapshotCreateFlags []string,
+	snapshotCreateEnv map[string]string,
+	parseSnapshotResultFn func(_ *testing.T, _, _ []string) parsedSnapshotResult,
+) int {
 	var numSuccessfulSnapshots int
 
 	changeFile, err := os.Stat(modifyEntry)
@@ -339,13 +365,13 @@ func() {
 
 			snapshotCreateWithArgs := append([]string{"snapshot", "create", source}, snapshotCreateFlags...)
 
-			_, errOut, runErr := e.Run(t, !expected.success, snapshotCreateWithArgs...)
+			stdOut, stdErr, runErr := e.Run(t, !expected.success, snapshotCreateWithArgs...)
 
 			if got, want := (runErr == nil), expected.success; got != want {
 				t.Fatalf("unexpected success %v, want %v", got, want)
 			}
 
-			parsed := parseSnapshotResult(t, errOut)
+			parsed := parseSnapshotResultFn(t, stdOut, stdErr)
 
 			if expected.success {
 				numSuccessfulSnapshots++
@@ -362,7 +388,7 @@ func() {
 			}
 
 			if got, want := parsed.partial, expected.wantPartial; got != want {
-				t.Fatalf("unexpected partial %v, want %v (%s)", got, want, errOut)
+				t.Fatalf("unexpected partial %v, want %v (%s)", got, want, stdErr)
 			}
 		}()
 	}
@@ -384,7 +410,7 @@ type parsedSnapshotResult struct {
 	ignoredErrorCount int
 }
 
-func parseSnapshotResult(t *testing.T, lines []string) parsedSnapshotResult {
+func parseSnapshotResultFromLog(t *testing.T, _, stdErr []string) parsedSnapshotResult {
 	t.Helper()
 
 	var (
@@ -392,7 +418,7 @@ func parseSnapshotResult(t *testing.T, lines []string) parsedSnapshotResult {
 		res parsedSnapshotResult
 	)
 
-	for _, l := range lines {
+	for _, l := range stdErr {
 		if match := createdSnapshotPattern.FindStringSubmatch(l); match != nil {
 			res.partial = strings.TrimSpace(match[1]) == "partial"
 			res.rootID = match[2]
@@ -416,3 +442,23 @@ func parseSnapshotResult(t *testing.T, lines []string) parsedSnapshotResult {
 
 	return res
 }
+
+func parseSnapshotResultJSON(t *testing.T, stdOut, _ []string) parsedSnapshotResult {
+	t.Helper()
+
+	if len(stdOut) == 0 {
+		return parsedSnapshotResult{}
+	}
+
+	var m snapshot.Manifest
+
+	testutil.MustParseJSONLines(t, stdOut, &m)
+
+	return parsedSnapshotResult{
+		manifestID:        string(m.ID),
+		rootID:            m.RootEntry.ObjectID.String(),
+		errorCount:        m.RootEntry.DirSummary.FatalErrorCount,
+		ignoredErrorCount: m.RootEntry.DirSummary.IgnoredErrorCount,
+		partial:           m.IncompleteReason != "",
+	}
+}