Repository: Shopify/themekit
Pull request #757 (2020-08-20 15:30:14)

Changing notification system
This implements @felipebrahm proposal from last week.

#### What this is doing
- removed notification functionality from the file watcher
- added the notification functionality to come after perform operations in the watch command
- implemented an adapter pattern for 
  - noop: no notify setting
  - url: for url as the notify setting
  - file: for a filepath as the notify setting
- Pass in a notification adapter to the watch command depending on the notification setting.

#### References:
- notify after http call https://github.com/Shopify/themekit/pull/750
- notify webhook https://github.com/felipebrahm/themekit/commit/538de9ddfab3f41b1345082effed633e2f11e62d
- Issue https://github.com/Shopify/themekit/issues/749

### Warn Checklist
- [x] 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.



Patch:
diff --git a/cmd/_testdata/notify_file b/cmd/_testdata/notify_file
new file mode 100644
index 0000000..e69de29
diff --git a/cmd/notify_adapters.go b/cmd/notify_adapters.go
new file mode 100644
index 0000000..7f9d1fb
--- /dev/null
+++ b/cmd/notify_adapters.go
@@ -0,0 +1,64 @@
+package cmd
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"net/url"
+	"os"
+	"time"
+
+	"github.com/Shopify/themekit/src/cmdutil"
+	"github.com/Shopify/themekit/src/colors"
+)
+
+type notifyAdapter interface {
+	notify(*cmdutil.Ctx, string)
+}
+
+func newNotifyAdapter(notifyPath string) notifyAdapter {
+	if notifyPath == "" {
+		return &noopNotify{}
+	} else if u, err := url.Parse(notifyPath); err == nil && u.Scheme != "" && u.Host != "" {
+		return &urlNotify{
+			url: notifyPath,
+			client: http.Client{
+				Timeout: time.Second,
+			},
+		}
+	}
+	return &fileNotify{path: notifyPath}
+}
+
+type noopNotify struct{}
+
+func (noop *noopNotify) notify(*cmdutil.Ctx, string) {}
+
+type urlNotify struct {
+	url    string
+	client http.Client
+}
+
+func (urlNote *urlNotify) notify(ctx *cmdutil.Ctx, path string) {
+	body, _ := json.Marshal(map[string]interface{}{"files": []string{path}})
+	resp, err := urlNote.client.Post(urlNote.url, "application/json", bytes.NewBuffer(body))
+	if err != nil {
+		ctx.Log.Printf(
+			`[%s] Error while notifying webhook "%s": %s`,
+			colors.Green(ctx.Env.Name),
+			colors.Blue(ctx.Env.Notify),
+			err,
+		)
+	} else {
+		resp.Body.Close()
+	}
+}
+
+type fileNotify struct {
+	path string
+}
+
+func (fileNote *fileNotify) notify(*cmdutil.Ctx, string) {
+	os.Create(fileNote.path)
+	os.Chtimes(fileNote.path, time.Now(), time.Now())
+}
diff --git a/cmd/theme.go b/cmd/theme.go
index b4d58ec..01539f9 100644
--- a/cmd/theme.go
+++ b/cmd/theme.go
@@ -90,7 +90,7 @@ func init() {
 	ThemeCmd.PersistentFlags().BoolVar(&flags.DisableIgnore, "no-ignore", false, "Will disable config ignores so that all files can be changed")
 	ThemeCmd.PersistentFlags().BoolVar(&flags.AllowLive, "allow-live", false, "Will allow themekit to make changes to the live theme on the store.")
 
-	watchCmd.Flags().StringVarP(&flags.NotifyFile, "notify", "n", "", "file to touch when workers have gone idle")
+	watchCmd.Flags().StringVarP(&flags.Notify, "notify", "n", "", "file to touch or url to notify when a file has been changed")
 	watchCmd.Flags().BoolVarP(&flags.AllEnvs, "allenvs", "a", false, "run command with all environments")
 	removeCmd.Flags().BoolVarP(&flags.AllEnvs, "allenvs", "a", false, "run command with all environments")
 	openCmd.Flags().BoolVarP(&flags.AllEnvs, "allenvs", "a", false, "run command with all environments")
diff --git a/cmd/watch.go b/cmd/watch.go
index 1e87efa..345b7c3 100644
--- a/cmd/watch.go
+++ b/cmd/watch.go
@@ -39,7 +39,6 @@ var watchCmd = &cobra.Command{
 			if err != nil {
 				return fmt.Errorf("[%s] Error while fetching info from server: %v", colors.Green(ctx.Env.Name), err)
 			}
-
 			for _, remoteAsset := range remoteFiles {
 				checksums[remoteAsset.Key] = remoteAsset.Checksum
 			}
@@ -50,14 +49,18 @@ var watchCmd = &cobra.Command{
 			}
 			watcher.Watch()
 			defer watcher.Stop()
+
 			signalChan := make(chan os.Signal)
 			signal.Notify(signalChan, os.Interrupt)
-			return watch(ctx, watcher.Events, signalChan)
+
+			notifier := newNotifyAdapter(ctx.Env.Notify)
+
+			return watch(ctx, watcher.Events, signalChan, notifier)
 		})
 	},
 }
 
-func watch(ctx *cmdutil.Ctx, events chan file.Event, sig chan os.Signal) error {
+func watch(ctx *cmdutil.Ctx, events chan file.Event, sig chan os.Signal, notifier notifyAdapter) error {
 	ctx.Flags.Verbose = true
 	ctx.Log.SetFlags(log.Ltime)
 
@@ -80,6 +83,9 @@ func watch(ctx *cmdutil.Ctx, events chan file.Event, sig chan os.Signal) error {
 			}
 			ctx.Log.Printf("[%s] processing %s", colors.Green(ctx.Env.Name), colors.Blue(event.Path))
 			perform(ctx, event.Path, event.Op, event.LastKnownChecksum)
+			if event.Op != file.Skip {
+				notifier.notify(ctx, event.Path)
+			}
 		case <-sig:
 			return nil
 		}
diff --git a/src/cmdutil/util.go b/src/cmdutil/util.go
index 8154b93..0b29a8f 100644
--- a/src/cmdutil/util.go
+++ b/src/cmdutil/util.go
@@ -43,7 +43,7 @@ type Flags struct {
 	IgnoredFiles          []string
 	Ignores               []string
 	DisableIgnore         bool
-	NotifyFile            string
+	Notify                string
 	AllEnvs               bool
 	Version               string
 	Prefix                string
@@ -234,7 +234,7 @@ func getFlagEnv(flags Flags) env.Env {
 		Domain:    flags.Domain,
 		Proxy:     flags.Proxy,
 		Timeout:   flags.Timeout,
-		Notify:    flags.NotifyFile,
+		Notify:    flags.Notify,
 	}
 
 	if !flags.DisableIgnore {
diff --git a/src/file/watcher.go b/src/file/watcher.go
index d0d6377..3e9e194 100644
--- a/src/file/watcher.go
+++ b/src/file/watcher.go
@@ -32,8 +32,6 @@ var (
 	// the interval that the watcher polls the filesystem this needs to be less than
 	// the drain timeout, otherwise debouncing will not work
 	pollInterval = 500 * time.Millisecond
-	// amount of time with no events before touching the notify file
-	idleTimeout = time.Second
 )
 
 // Event decsribes a file change event
@@ -50,7 +48,6 @@ type Watcher struct {
 	Events chan Event
 
 	fsWatcher *watcher.Watcher
-	notify    string
 	directory string
 	checksums map[string]string
 }
@@ -76,7 +73,6 @@ func NewWatcher(e *env.Env, configPath string, checksums map[string]string) (*Wa
 		Events:    make(chan Event),
 		directory: e.Directory,
 		checksums: checksums,
-		notify:    e.Notify,
 		fsWatcher: fsWatcher,
 	}, nil
 }
@@ -102,16 +98,12 @@ func (w *Watcher) Watch() {
 }
 
 func (w *Watcher) watchFsEvents() {
-	idleTimer := time.NewTimer(idleTimeout)
-	defer idleTimer.Stop()
 	for {
 		select {
 		case event, ok := <-w.fsWatcher.Event:
-			if ok && w.onEvent(event) {
-				idleTimer.Reset(idleTimeout)
+			if ok {
+				w.onEvent(event)
 			}
-		case <-idleTimer.C:
-			w.onIdle()
 		case <-w.fsWatcher.Closed:
 			w.Stop()
 			return
@@ -204,14 +196,6 @@ func (w *Watcher) Stop() {
 	}
 }
 
-func (w *Watcher) onIdle() {
-	if w.notify == "" {
-		return
-	}
-	os.Create(w.notify)
-	os.Chtimes(w.notify, time.Now(), time.Now())
-}
-
 func fileChecksum(dir, src string) (string, error) {
 	sum := md5.New()
 	s, err := os.Open(filepath.Join(dir, src))


Test patch:
diff --git a/cmd/notify_adapters_test.go b/cmd/notify_adapters_test.go
new file mode 100644
index 0000000..ea4b492
--- /dev/null
+++ b/cmd/notify_adapters_test.go
@@ -0,0 +1,78 @@
+package cmd
+
+import (
+	"encoding/json"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewNotifyAdapter(t *testing.T) {
+	adapter := newNotifyAdapter("")
+	_, ok := adapter.(*noopNotify)
+	assert.True(t, ok)
+
+	adapter = newNotifyAdapter("note.txt")
+	_, ok = adapter.(*fileNotify)
+	assert.True(t, ok)
+
+	adapter = newNotifyAdapter("http://localhost:3000/notify")
+	_, ok = adapter.(*urlNotify)
+	assert.True(t, ok)
+}
+
+func TestNoopAdapter(t *testing.T) {
+	adapter := newNotifyAdapter("")
+	ctx, _, _, _, _ := createTestCtx()
+	adapter.notify(ctx, "")
+}
+
+func TestNotifyFile(t *testing.T) {
+	notifyPath := filepath.Join("_testdata", "notify_file")
+	adapter := newNotifyAdapter(notifyPath)
+	ctx, _, _, _, _ := createTestCtx()
+
+	os.Remove(notifyPath)
+	_, err := os.Stat(notifyPath)
+	assert.True(t, os.IsNotExist(err))
+
+	adapter.notify(ctx, "")
+	_, err = os.Stat(notifyPath)
+	assert.Nil(t, err)
+	// need to make the time different larger than milliseconds because windows
+	// trucates the time and it will fail
+	os.Chtimes(notifyPath, time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, -1))
+	info1, err := os.Stat(notifyPath)
+	assert.Nil(t, err)
+
+	adapter.notify(ctx, "")
+	info2, err := os.Stat(notifyPath)
+	assert.Nil(t, err)
+	assert.NotEqual(t, info1.ModTime(), info2.ModTime())
+}
+
+func TestNotifyURL(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		assert.Equal(t, r.Header.Get("Content-Type"), "application/json")
+		reqBody, err := ioutil.ReadAll(r.Body)
+		assert.Nil(t, err)
+		data := map[string]interface{}{}
+		assert.Nil(t, json.Unmarshal(reqBody, &data))
+		assert.NotNil(t, data["files"])
+		files, _ := data["files"].([]interface{})
+		assert.Equal(t, 1, len(files))
+		assert.Equal(t, "assets/app.js", files[0])
+	}))
+	defer server.Close()
+
+	ctx, _, _, _, _ := createTestCtx()
+
+	adapter := newNotifyAdapter(server.URL)
+	adapter.notify(ctx, "assets/app.js")
+}
diff --git a/cmd/watch_test.go b/cmd/watch_test.go
index b20505e..9125dc2 100644
--- a/cmd/watch_test.go
+++ b/cmd/watch_test.go
@@ -8,14 +8,21 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 
+	"github.com/Shopify/themekit/src/cmdutil"
 	"github.com/Shopify/themekit/src/file"
 	"github.com/Shopify/themekit/src/shopify"
 )
 
+type testAdapter struct{ mock.Mock }
+
+func (adapter *testAdapter) notify(ctx *cmdutil.Ctx, path string) {
+	adapter.Called(ctx, path)
+}
+
 func TestWatch(t *testing.T) {
 	ctx, _, _, _, _ := createTestCtx()
 	ctx.Env.ReadOnly = true
-	err := watch(ctx, make(chan file.Event), make(chan os.Signal))
+	err := watch(ctx, make(chan file.Event), make(chan os.Signal), nil)
 	if assert.NotNil(t, err) {
 		assert.Contains(t, err.Error(), "environment is reaonly")
 	}
@@ -24,7 +31,7 @@ func TestWatch(t *testing.T) {
 	ctx, _, _, stdOut, _ := createTestCtx()
 	ctx.Flags.ConfigPath = "config.yml"
 	eventChan <- file.Event{Path: ctx.Flags.ConfigPath}
-	err = watch(ctx, eventChan, make(chan os.Signal))
+	err = watch(ctx, eventChan, make(chan os.Signal), nil)
 	if assert.NotNil(t, err) {
 		assert.Contains(t, err.Error(), "reload")
 	}
@@ -39,11 +46,14 @@ func TestWatch(t *testing.T) {
 		eventChan <- file.Event{Op: file.Update, Path: "assets/app.js"}
 		signalChan <- os.Interrupt
 	}()
-	err = watch(ctx, eventChan, signalChan)
+	notifier := new(testAdapter)
+	notifier.On("notify", ctx, "assets/app.js")
+	err = watch(ctx, eventChan, signalChan, notifier)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Watching for file changes")
 	assert.Contains(t, stdOut.String(), "processing assets/app.js")
 	assert.Contains(t, stdErr.String(), "error loading assets/app.js: readAsset: ")
+	notifier.AssertExpectations(t)
 
 	signalChan = make(chan os.Signal)
 	eventChan = make(chan file.Event)
@@ -55,11 +65,14 @@ func TestWatch(t *testing.T) {
 		eventChan <- file.Event{Op: file.Update, Path: "assets/app.js"}
 		signalChan <- os.Interrupt
 	}()
-	err = watch(ctx, eventChan, signalChan)
+	notifier = new(testAdapter)
+	notifier.On("notify", ctx, "assets/app.js")
+	err = watch(ctx, eventChan, signalChan, notifier)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Watching for file changes")
 	assert.Contains(t, stdOut.String(), "processing assets/app.js")
 	assert.Contains(t, stdOut.String(), "Updated assets/app.js")
+	notifier.AssertExpectations(t)
 
 	signalChan = make(chan os.Signal)
 	eventChan = make(chan file.Event)
@@ -71,11 +84,14 @@ func TestWatch(t *testing.T) {
 		eventChan <- file.Event{Op: file.Remove, Path: "assets/app.js"}
 		signalChan <- os.Interrupt
 	}()
-	err = watch(ctx, eventChan, signalChan)
+	notifier = new(testAdapter)
+	notifier.On("notify", ctx, "assets/app.js")
+	err = watch(ctx, eventChan, signalChan, notifier)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Watching for file changes")
 	assert.Contains(t, stdOut.String(), "processing assets/app.js")
 	assert.Contains(t, stdOut.String(), "Deleted assets/app.js")
+	notifier.AssertExpectations(t)
 }
 
 func TestPerform(t *testing.T) {
diff --git a/src/cmdutil/util_test.go b/src/cmdutil/util_test.go
index 1609878..8b8eef1 100644
--- a/src/cmdutil/util_test.go
+++ b/src/cmdutil/util_test.go
@@ -125,7 +125,7 @@ func TestGetFlagEnv(t *testing.T) {
 		Domain:       "o",
 		Proxy:        "r",
 		Timeout:      1,
-		NotifyFile:   "n",
+		Notify:       "n",
 		IgnoredFiles: []string{"i"},
 		Ignores:      []string{"c"},
 	}
diff --git a/src/file/watcher_test.go b/src/file/watcher_test.go
index ec23940..84870c8 100644
--- a/src/file/watcher_test.go
+++ b/src/file/watcher_test.go
@@ -22,7 +22,6 @@ func TestMain(m *testing.M) {
 
 func TestNewFileWatcher(t *testing.T) {
 	w := createTestWatcher(t)
-	assert.Equal(t, w.notify, "/tmp/notifytest")
 	assert.NotNil(t, w.fsWatcher)
 	assert.NotNil(t, w.Events)
 }
@@ -218,34 +217,10 @@ func TestFileWatcher_StopWatching(t *testing.T) {
 	w.Stop()
 }
 
-func TestFileWatcher_TouchNotifyFile(t *testing.T) {
-	notifyPath := filepath.Join("_testdata", "notify_file")
-	w, _ := NewWatcher(&env.Env{Notify: notifyPath}, "", map[string]string{})
-
-	os.Remove(notifyPath)
-	_, err := os.Stat(notifyPath)
-	assert.True(t, os.IsNotExist(err))
-
-	w.onIdle()
-	_, err = os.Stat(notifyPath)
-	assert.Nil(t, err)
-	// need to make the time different larger than milliseconds because windows
-	// trucates the time and it will fail
-	os.Chtimes(w.notify, time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, -1))
-	info1, err := os.Stat(notifyPath)
-	assert.Nil(t, err)
-
-	w.onIdle()
-	info2, err := os.Stat(notifyPath)
-	assert.Nil(t, err)
-	assert.NotEqual(t, info1.ModTime(), info2.ModTime())
-}
-
 func createTestWatcher(t *testing.T) *Watcher {
 	e := &env.Env{
 		Directory:    filepath.Join("_testdata", "project"),
 		IgnoredFiles: []string{"config/"},
-		Notify:       "/tmp/notifytest",
 	}
 	w, err := NewWatcher(e, filepath.Join("_testdata", "project", "config.yml"), map[string]string{})
 	assert.Nil(t, err)
@@ -256,7 +231,6 @@ func createTestFilterHook(t *testing.T) watcher.FilterFileHookFunc {
 	e := &env.Env{
 		Directory:    filepath.Join("_testdata", "project"),
 		IgnoredFiles: []string{"config/"},
-		Notify:       "/tmp/notifytest",
 	}
 	hook, err := filterHook(e, filepath.Join("_testdata", "project", "config.yml"))
 	assert.Nil(t, err)