Merge pull request #826 from Shopify/action_summary_and_return_status

Action summary and return status

changelog.txt

@@ -1,3 +1,9 @@
+v1.1.3
+=====================
+- Added an error summary (#826)
+- Added error count to action summary (#826)
+- Set status error codes when errors occur for better scriptability (#826)
+
 v1.1.2 (Sep 29, 2020)
 =====================
 - Fix hanging uploads (#809)

cmd/deploy.go

@@ -48,6 +48,8 @@ var deployCmd = &cobra.Command{
 	RunE: func(cmd *cobra.Command, args []string) error {
 		return cmdutil.ForEachClient(flags, args, deploy)
 	},
+	PostRun: func(cmd *cobra.Command, args []string) {
+	},
 }
 
 func deploy(ctx *cmdutil.Ctx) error {
@@ -60,16 +62,6 @@ func deploy(ctx *cmdutil.Ctx) error {
 		return err
 	}
 
-	updateCount := 0
-	skipCount := 0
-	removeCount := 0
-
-	defer func() {
-		if ctx.Flags.Verbose {
-			fmt.Printf("Updated: %d, Removed: %d, No Changes: %d\n", updateCount, removeCount, skipCount)
-		}
-	}()
-
 	var deployGroup sync.WaitGroup
 	ctx.StartProgress(len(assetsActions))
 	for path, op := range assetsActions {
@@ -82,18 +74,10 @@ func deploy(ctx *cmdutil.Ctx) error {
 			defer deployGroup.Done()
 			perform(ctx, path, op, "")
 		}(path, op)
-
-		switch op {
-		case file.Update:
-			updateCount++
-		case file.Skip:
-			skipCount++
-		case file.Remove:
-			removeCount++
-		}
 	}
 
 	deployGroup.Wait()
+
 	return nil
 }
 

cmd/download.go

@@ -4,20 +4,14 @@ import (
 	"fmt"
 	"path/filepath"
 	"sync"
-	"sync/atomic"
 
 	"github.com/spf13/cobra"
 
 	"github.com/Shopify/themekit/src/cmdutil"
-	"github.com/Shopify/themekit/src/colors"
+	"github.com/Shopify/themekit/src/file"
 	"github.com/Shopify/themekit/src/shopify"
 )
 
-var (
-	skipCount  int32
-	errorCount int32
-)
-
 var downloadCmd = &cobra.Command{
 	Use:   "download <filenames>",
 	Short: "Download one or all of the theme files",
@@ -47,69 +41,45 @@ func download(ctx *cmdutil.Ctx) error {
 	}
 
 	ctx.StartProgress(len(assets))
-	for _, asset := range assets {
+	for asset, op := range assets {
 		downloadGroup.Add(1)
-		go func(requestAsset shopify.Asset) {
-			defer ctx.DoneTask()
+		go func(path string, op file.Op) {
 			defer downloadGroup.Done()
-
-			localAsset, _ := shopify.ReadAsset(ctx.Env, requestAsset.Key)
-			if localAsset.Checksum == requestAsset.Checksum && requestAsset.Checksum != "" {
-				atomic.AddInt32(&skipCount, 1)
-				if ctx.Flags.Verbose {
-					ctx.Log.Printf("[%s] No Changes %s (%s)", colors.Green(ctx.Env.Name), colors.Blue(requestAsset.Key), localAsset.Checksum)
-				}
-			} else if asset, err := ctx.Client.GetAsset(requestAsset.Key); err != nil {
-				ctx.Err("[%s] error downloading %s: %s", colors.Green(ctx.Env.Name), colors.Blue(requestAsset.Key), err)
-				atomic.AddInt32(&errorCount, 1)
-			} else if err = asset.Write(ctx.Env.Directory); err != nil {
-				atomic.AddInt32(&errorCount, 1)
-				ctx.Err("[%s] error writing %s: %s", colors.Green(ctx.Env.Name), colors.Blue(requestAsset.Key), err)
-			} else if ctx.Flags.Verbose {
-				var checksumOutput = ""
-				if asset.Checksum != "" {
-					checksumOutput = "Remote: " + requestAsset.Checksum + ", Local: " + localAsset.Checksum
-				} else {
-					checksumOutput = "Local: " + localAsset.Checksum
-				}
-				ctx.Log.Printf("[%s] Successfully wrote %s to disk (%s)", colors.Green(ctx.Env.Name), colors.Blue(asset.Key), checksumOutput)
-			}
-		}(asset)
+			perform(ctx, path, op, "")
+		}(asset, op)
 	}
 
 	downloadGroup.Wait()
-	downloadCount := int32(len(assets)) - skipCount - errorCount
-	defer func() {
-		if ctx.Flags.Verbose {
-			ctx.Log.Printf("Downloaded: %d, No Changes: %d, Errored: %d", downloadCount, skipCount, errorCount)
-		}
-	}()
+
 	return nil
 }
 
-func filesToDownload(ctx *cmdutil.Ctx) ([]shopify.Asset, error) {
+func filesToDownload(ctx *cmdutil.Ctx) (map[string]file.Op, error) {
+	fetchableFiles := map[string]file.Op{}
+
 	assets, err := ctx.Client.GetAllAssets()
 	if err != nil {
-		return []shopify.Asset{}, err
+		return fetchableFiles, err
 	}
 
 	if len(ctx.Args) <= 0 {
-		return assets, nil
+		for _, asset := range assets {
+			fetchableFiles[asset.Key] = downloadFileAction(ctx, asset)
+		}
+		return fetchableFiles, nil
 	}
 
-	fetchableFiles := []shopify.Asset{}
 	for _, asset := range assets {
 		for _, pattern := range ctx.Args {
 			// These need to be converted to platform specific because filepath.Match
 			// uses platform specific separators
 			pattern = filepath.FromSlash(pattern)
 			filename := filepath.FromSlash(asset.Key)
-
 			globMatched, _ := filepath.Match(pattern, filename)
 			dirMatched, _ := filepath.Match(pattern+string(filepath.Separator)+"*", filename)
 			fileMatched := filename == pattern
 			if globMatched || dirMatched || fileMatched {
-				fetchableFiles = append(fetchableFiles, asset)
+				fetchableFiles[asset.Key] = downloadFileAction(ctx, asset)
 			}
 		}
 	}
@@ -120,3 +90,14 @@ func filesToDownload(ctx *cmdutil.Ctx) ([]shopify.Asset, error) {
 
 	return fetchableFiles, nil
 }
+
+func downloadFileAction(ctx *cmdutil.Ctx, asset shopify.Asset) file.Op {
+	op := file.Get
+	if asset.Checksum == "" {
+		return op
+	}
+	if localAsset, _ := shopify.ReadAsset(ctx.Env, asset.Key); asset.Checksum == localAsset.Checksum {
+		op = file.Skip
+	}
+	return op
+}

cmd/download_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 
+	"github.com/Shopify/themekit/src/file"
 	"github.com/Shopify/themekit/src/shopify"
 )
 
@@ -24,7 +25,9 @@ func TestDownload(t *testing.T) {
 
 	ctx, client, _, _, stdErr := createTestCtx()
 	client.On("GetAllAssets").Return(allAssets, nil)
-	client.On("GetAsset", mock.MatchedBy(func(string) bool { return true })).Return(shopify.Asset{}, nil).Times(len(allAssets))
+	client.On("GetAsset", mock.MatchedBy(func(string) bool { return true })).
+		Return(shopify.Asset{Key: "assets/logo.png"}, nil).
+		Times(len(allAssets))
 	err := download(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdErr.String(), "error writing assets/logo.png")
@@ -45,19 +48,23 @@ func TestDownload(t *testing.T) {
 
 func TestFilesToDownload(t *testing.T) {
 	allAssets := []shopify.Asset{{Key: "assets/logo.png"}, {Key: "templates/customers/test.liquid"}, {Key: "config/test.liquid"}, {Key: "layout/test.liquid"}, {Key: "snippets/test.liquid"}, {Key: "templates/test.liquid"}, {Key: "locales/test.liquid"}, {Key: "sections/test.liquid"}}
+	allOps := map[string]file.Op{}
+	for _, asset := range allAssets {
+		allOps[asset.Key] = file.Get
+	}
 
 	testcases := []struct {
 		err     string
 		respErr error
 		args    []string
-		ret     []shopify.Asset
+		ret     map[string]file.Op
 	}{
-		{ret: allAssets},
-		{args: []string{"assets/logo.png"}, ret: []shopify.Asset{{Key: "assets/logo.png"}}},
-		{args: []string{"assets/*"}, ret: []shopify.Asset{{Key: "assets/logo.png"}}},
-		{args: []string{"templates"}, ret: []shopify.Asset{{Key: "templates/test.liquid"}}},
-		{args: []string{"assets/nope.png"}, ret: []shopify.Asset{}, err: "No file paths matched the inputted arguments"},
-		{args: []string{"assets/nope.png"}, ret: []shopify.Asset{}, respErr: fmt.Errorf("server error"), err: "server error"},
+		{ret: allOps},
+		{args: []string{"assets/logo.png"}, ret: map[string]file.Op{"assets/logo.png": file.Get}},
+		{args: []string{"assets/*"}, ret: map[string]file.Op{"assets/logo.png": file.Get}},
+		{args: []string{"templates"}, ret: map[string]file.Op{"templates/test.liquid": file.Get}},
+		{args: []string{"assets/nope.png"}, ret: map[string]file.Op{}, err: "No file paths matched the inputted arguments"},
+		{args: []string{"assets/nope.png"}, ret: map[string]file.Op{}, respErr: fmt.Errorf("server error"), err: "server error"},
 	}
 
 	for i, testcase := range testcases {
@@ -73,3 +80,18 @@ func TestFilesToDownload(t *testing.T) {
 		}
 	}
 }
+
+func TestFilesToDownloadFileAction(t *testing.T) {
+	ctx, _, _, _, _ := createTestCtx()
+	ctx.Env.Directory = "_testdata/projectdir"
+
+	localAsset, _ := shopify.ReadAsset(ctx.Env, "assets/app.js")
+	op := downloadFileAction(ctx, shopify.Asset{Key: "assets/app.js", Checksum: localAsset.Checksum})
+	assert.Equal(t, file.Skip, op)
+
+	op = downloadFileAction(ctx, shopify.Asset{Key: "assets/app.js", Checksum: "not correct"})
+	assert.Equal(t, file.Get, op)
+
+	op = downloadFileAction(ctx, shopify.Asset{Key: "assets/app.js"})
+	assert.Equal(t, file.Get, op)
+}

cmd/theme.go

@@ -103,5 +103,17 @@ func init() {
 	getCmd.Flags().BoolVarP(&flags.List, "list", "l", false, "list available themes.")
 	deployCmd.Flags().BoolVarP(&flags.NoDelete, "nodelete", "n", false, "do not delete files on shopify during deploy.")
 
-	ThemeCmd.AddCommand(publishCmd, openCmd, versionCmd, newCmd, configureCmd, downloadCmd, removeCmd, updateCmd, watchCmd, getCmd, deployCmd)
+	ThemeCmd.AddCommand(
+		publishCmd,
+		openCmd,
+		versionCmd,
+		newCmd,
+		configureCmd,
+		downloadCmd,
+		removeCmd,
+		updateCmd,
+		watchCmd,
+		getCmd,
+		deployCmd,
+	)
 }

cmd/theme/main.go

@@ -27,7 +27,7 @@ func main() {
 	}
 
 	if err := cmd.ThemeCmd.Execute(); err != nil {
-		stdErr.Fatal(err)
+		stdErr.Fatal(colors.Red(err.Error()))
 	}
 
 	if memProfile := os.Getenv(memProfileVar); memProfile != "" {

cmd/watch.go

@@ -34,6 +34,8 @@ var watchCmd = &cobra.Command{
  `,
 	RunE: func(cmd *cobra.Command, args []string) error {
 		return cmdutil.ForEachClient(flags, args, func(ctx *cmdutil.Ctx) error {
+			ctx.DisableSummary()
+
 			checksums := map[string]string{}
 			remoteFiles, err := ctx.Client.GetAllAssets()
 			if err != nil {
@@ -94,22 +96,29 @@ func watch(ctx *cmdutil.Ctx, events chan file.Event, sig chan os.Signal, notifie
 }
 
 func perform(ctx *cmdutil.Ctx, path string, op file.Op, checksum string) {
-	defer ctx.DoneTask()
+	defer ctx.DoneTask(op)
 
 	switch op {
 	case file.Skip:
-		localAsset, _ := shopify.ReadAsset(ctx.Env, path)
-
 		if ctx.Flags.Verbose {
+			localAsset, _ := shopify.ReadAsset(ctx.Env, path)
 			checksumOutput := "Checksum: " + localAsset.Checksum
-			ctx.Log.Printf("[%s] %s %s (%s)", colors.Green(ctx.Env.Name), colors.BrightBlack("Skipped"), colors.Blue(path), checksumOutput)
+			ctx.Log.Printf("[%s] %s %s (%s)", colors.Green(ctx.Env.Name), colors.Cyan("Skipped"), colors.Blue(path), checksumOutput)
 		}
 	case file.Remove:
 		if err := ctx.Client.DeleteAsset(shopify.Asset{Key: path}); err != nil {
 			ctx.Err("[%s] (%s) %s", colors.Green(ctx.Env.Name), colors.Blue(path), err)
 		} else if ctx.Flags.Verbose {
 			ctx.Log.Printf("[%s] Deleted %s", colors.Green(ctx.Env.Name), colors.Blue(path))
 		}
+	case file.Get:
+		if asset, err := ctx.Client.GetAsset(path); err != nil {
+			ctx.Err("[%s] error downloading %s: %s", colors.Green(ctx.Env.Name), colors.Blue(path), err)
+		} else if err = asset.Write(ctx.Env.Directory); err != nil {
+			ctx.Err("[%s] error writing %s: %s", colors.Green(ctx.Env.Name), colors.Blue(asset.Key), err)
+		} else if ctx.Flags.Verbose {
+			ctx.Log.Printf("[%s] Successfully wrote %s to disk", colors.Green(ctx.Env.Name), colors.Blue(asset.Key))
+		}
 	default:
 		assetLimitSemaphore <- struct{}{}
 		defer func() { <-assetLimitSemaphore }()
@@ -120,7 +129,7 @@ func perform(ctx *cmdutil.Ctx, path string, op file.Op, checksum string) {
 			return
 		}
 
-		if err := ctx.Client.UpdateAsset(asset, checksum); err != nil {
+		if err = ctx.Client.UpdateAsset(asset, checksum); err != nil {
 			ctx.Err("[%s] (%s) %s", colors.Green(ctx.Env.Name), colors.Blue(asset.Key), err)
 		} else if ctx.Flags.Verbose {
 			ctx.Log.Printf("[%s] Updated %s", colors.Green(ctx.Env.Name), colors.Blue(asset.Key))

src/cmdutil/summary.go

@@ -0,0 +1,71 @@
+package cmdutil
+
+import (
+	"fmt"
+	"strings"
+	"sync/atomic"
+
+	"github.com/Shopify/themekit/src/colors"
+	"github.com/Shopify/themekit/src/file"
+)
+
+type cmdSummary struct {
+	actions, downloaded, uploaded, skipped, removed int32
+	disabled                                        bool
+	errors                                          []string
+}
+
+func (sum *cmdSummary) completeOp(op file.Op) {
+	atomic.AddInt32(&sum.actions, 1)
+	switch op {
+	case file.Update:
+		atomic.AddInt32(&sum.uploaded, 1)
+	case file.Skip:
+		atomic.AddInt32(&sum.skipped, 1)
+	case file.Remove:
+		atomic.AddInt32(&sum.removed, 1)
+	case file.Get:
+		atomic.AddInt32(&sum.downloaded, 1)
+	}
+}
+
+func (sum *cmdSummary) disable() {
+	sum.disabled = true
+}
+
+func (sum *cmdSummary) err(errStr string) {
+	sum.errors = append(sum.errors, errStr)
+}
+
+func (sum *cmdSummary) hasErrors() bool {
+	return !sum.disabled && len(sum.errors) > 0
+}
+
+func (sum *cmdSummary) display(ctx *Ctx) {
+	if sum.disabled || sum.actions == 0 {
+		return
+	}
+	var results = []string{fmt.Sprintf("%v files", sum.actions)}
+	if sum.downloaded > 0 {
+		results = append(results, fmt.Sprintf("%v: %v", colors.Blue("Downloaded"), sum.downloaded))
+	}
+	if sum.uploaded > 0 {
+		results = append(results, fmt.Sprintf("%v: %v", colors.Green("Updated"), sum.uploaded))
+	}
+	if sum.removed > 0 {
+		results = append(results, fmt.Sprintf("%v: %v", colors.Yellow("Removed"), sum.removed))
+	}
+	if sum.skipped > 0 {
+		results = append(results, fmt.Sprintf("%v: %v", colors.Cyan("No Change"), sum.skipped))
+	}
+	if len(sum.errors) > 0 {
+		results = append(results, fmt.Sprintf("%v: %v", colors.Red("Errored"), len(sum.errors)))
+	}
+	ctx.Log.Printf("[%v] %v", colors.Green(ctx.Env.Name), strings.Join(results, ", "))
+	if len(sum.errors) > 0 {
+		ctx.ErrLog.Printf("[%s] %s", colors.Green(ctx.Env.Name), colors.Red("Errors encountered: "))
+		for _, msg := range sum.errors {
+			ctx.ErrLog.Printf("\t%v", msg)
+		}
+	}
+}