Repository: Shopify/themekit
Pull request #724 (2020-06-29 15:03:54)

Skip uploads if checksums match
Co-authored with @ayronshopify 

Depends on this Core change: https://github.com/Shopify/shopify/pull/251785

TODO:

- [ ] squash commits after final approval

This PR adds behaviour so that if a files local checksum matches the remote checksum, then we skip it.

The existing upload process had two variants:
- If no list of files is provided to `deploy`, it will first make a GET request to discover what files are on the server. From this it can determine if any local files should be deleted.
- If a list of files it provided to `deploy`, then it does not make the GET request. 

To compare local checksums against remote checksums, we now need to make the GET request for both these situations.

Note: There are also fixes to no longer show skipped files when in non-verbose mode.

- [x] Verify how `watch` behaves now.

### Warn Checklist
- [ ] This changes the interface and requires a Major/Minor version change.
- [x] I have :tophat:'d these changes by using the commands I changed by hand.
- [ ] ~I have added a dependancy to the project.~ No new dependencies



Patch:
diff --git a/cmd/deploy.go b/cmd/deploy.go
index 0924292..85944ad 100644
--- a/cmd/deploy.go
+++ b/cmd/deploy.go
@@ -80,15 +80,17 @@ func deploy(ctx *cmdutil.Ctx) error {
 
 func generateActions(ctx *cmdutil.Ctx) (map[string]file.Op, error) {
 	assetsActions := map[string]file.Op{}
+	pathsToChecksums := map[string]string{}
 
-	if len(ctx.Args) == 0 && !ctx.Flags.NoDelete {
-		remoteFiles, err := ctx.Client.GetAllAssets()
-		if err != nil {
-			return assetsActions, err
-		}
-		for _, asset := range remoteFiles {
-			assetsActions[asset.Key] = file.Remove
+	remoteFiles, err := ctx.Client.GetAllAssets()
+	if err != nil {
+		return assetsActions, err
+	}
+	for _, remoteAsset := range remoteFiles {
+		if len(ctx.Args) == 0 && !ctx.Flags.NoDelete {
+			assetsActions[remoteAsset.Key] = file.Remove
 		}
+		pathsToChecksums[remoteAsset.Key] = remoteAsset.Checksum
 	}
 
 	localAssets, err := shopify.FindAssets(ctx.Env, ctx.Args...)
@@ -103,7 +105,11 @@ func generateActions(ctx *cmdutil.Ctx) (map[string]file.Op, error) {
 
 	for _, asset := range localAssets {
 		var path = asset.Key
-		assetsActions[path] = file.Update
+		if asset.Checksum != "" && (asset.Checksum == pathsToChecksums[asset.Key]) {
+			assetsActions[path] = file.Skip
+		} else {
+			assetsActions[path] = file.Update
+		}
 	}
 	return assetsActions, nil
 }
diff --git a/cmd/download.go b/cmd/download.go
index 298302d..b8a1f21 100644
--- a/cmd/download.go
+++ b/cmd/download.go
@@ -47,7 +47,9 @@ func download(ctx *cmdutil.Ctx) error {
 
 			localAsset, _ := shopify.ReadAsset(ctx.Env, requestAsset.Key)
 			if localAsset.Checksum == requestAsset.Checksum && requestAsset.Checksum != "" {
-				ctx.Log.Printf("[%s] Skipped %s (%s)", colors.Green(ctx.Env.Name), colors.Blue(requestAsset.Key), localAsset.Checksum)
+				if ctx.Flags.Verbose {
+			  	ctx.Log.Printf("[%s] Skipped %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)
 			} else if err = asset.Write(ctx.Env.Directory); err != nil {
diff --git a/cmd/watch.go b/cmd/watch.go
index 6246fa8..96344d7 100644
--- a/cmd/watch.go
+++ b/cmd/watch.go
@@ -79,13 +79,21 @@ func watch(ctx *cmdutil.Ctx, events chan file.Event, sig chan os.Signal) error {
 func perform(ctx *cmdutil.Ctx, path string, op file.Op) {
 	defer ctx.DoneTask()
 
-	if op == file.Remove {
+	switch op {
+	case file.Skip:
+		localAsset, _ := shopify.ReadAsset(ctx.Env, path)
+
+		if ctx.Flags.Verbose {
+			checksumOutput := "Checksum: " + localAsset.Checksum
+			ctx.Log.Printf("[%s] Skipping %s (%s)", colors.Green(ctx.Env.Name), 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))
 		}
-	} else {
+	default:
 		assetLimitSemaphore <- struct{}{}
 		defer func() { <-assetLimitSemaphore }()
 
diff --git a/src/file/watcher.go b/src/file/watcher.go
index a4c8119..0cd5681 100644
--- a/src/file/watcher.go
+++ b/src/file/watcher.go
@@ -18,6 +18,8 @@ const (
 	Update Op = iota
 	// Remove is a file op where the file is removed
 	Remove
+	// Skip is a file op where the remote file matches the local file so is not transferred
+	Skip
 	filepathSplit = " -> "
 )
 


Test patch:
diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go
index 42d46c1..b8460d8 100644
--- a/cmd/deploy_test.go
+++ b/cmd/deploy_test.go
@@ -14,41 +14,76 @@ import (
 	"github.com/Shopify/themekit/src/shopify"
 )
 
-func TestUpload(t *testing.T) {
+func TestUploadSingleFile(t *testing.T) {
 	ctx, client, _, _, _ := createTestCtx()
 	ctx.Args = []string{"templates/layout.liquid"}
 	ctx.Flags.NoDelete = true
+	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "templates/layout.liquid"}}, nil)
 	client.On("UpdateAsset", shopify.Asset{Key: "templates/layout.liquid"}).Return(nil)
 	err := deploy(ctx)
 	assert.NotNil(t, err)
+}
 
-	ctx, client, _, _, _ = createTestCtx()
+func TestUploadWithReadOnlyOption(t *testing.T) {
+	ctx, client, _, _, _ := createTestCtx()
 	ctx.Args = []string{"templates/layout.liquid"}
 	ctx.Flags.NoDelete = true
 	ctx.Env.ReadOnly = true
 	client.On("UpdateAsset", shopify.Asset{Key: "templates/layout.liquid"}).Return(nil)
-	err = deploy(ctx)
+	err := deploy(ctx)
 	if assert.NotNil(t, err) {
 		assert.Contains(t, err.Error(), "environment is readonly")
 	}
+}
 
+func TestUploadWithVerboseOptions(t *testing.T) {
 	ctx, client, _, stdOut, _ := createTestCtx()
 	ctx.Args = []string{"assets/app.js"}
 	ctx.Flags.NoDelete = true
 	ctx.Env.Directory = "_testdata/projectdir"
 	ctx.Flags.Verbose = true
+	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "assets/app.js"}}, nil)
 	// This checksum corresponds to a zero-byte file
 	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
-	err = deploy(ctx)
+	err := deploy(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Updated assets/app.js")
+}
 
-	ctx, client, _, stdOut, _ = createTestCtx()
+func TestUploadAllFiles(t *testing.T) {
+	ctx, client, _, stdOut, _ := createTestCtx()
 	ctx.Env.Directory = "_testdata/projectdir"
 	ctx.Flags.Verbose = true
 	ctx.Flags.NoDelete = true
+	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "config/settings_data.json"}}, nil)
 	client.On("UpdateAsset", mock.MatchedBy(func(a shopify.Asset) bool { return true })).Return(nil)
-	err = deploy(ctx)
+	err := deploy(ctx)
+	assert.Nil(t, err)
+	assert.Contains(t, stdOut.String(), "Updated config/settings_data.json")
+}
+
+func TestUploadForSkipFileWhenChecksumsMatch(t *testing.T) {
+	ctx, client, _, stdOut, _ := createTestCtx()
+	ctx.Env.Directory = "_testdata/projectdir"
+	ctx.Flags.Verbose = true
+	ctx.Flags.NoDelete = true
+	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "config/settings_data.json", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}}, nil)
+	// the _testdirectory contains two assets. We expect one to be uploaded, one to be skipped.
+	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
+	err := deploy(ctx)
+	assert.Nil(t, err)
+	assert.Contains(t, stdOut.String(), "Skipping config/settings_data.json")
+	assert.Contains(t, stdOut.String(), "Updated assets/app.js")
+}
+
+func TestUploadForDoNotSkipWhenChecksumsDiffer(t *testing.T) {
+	ctx, client, _, stdOut, _ := createTestCtx()
+	ctx.Env.Directory = "_testdata/projectdir"
+	ctx.Flags.Verbose = true
+	ctx.Flags.NoDelete = true
+	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "config/settings_data.json", Checksum: "abc123"}}, nil)
+	client.On("UpdateAsset", mock.MatchedBy(func(a shopify.Asset) bool { return true })).Return(nil)
+	err := deploy(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Updated config/settings_data.json")
 }