Merge pull request #465 from Shopify/manifest_changed_only_on_action

Changing how the manifest deals with diff dates

cmd/bootstrap_test.go

@@ -126,11 +126,11 @@ func TestSaveConfiguration(t *testing.T) {
 
 	kittest.GenerateConfig("example.myshopify.io", true)
 	env, _ := kit.LoadEnvironments("config.yml")
-	config, _ := env.GetConfiguration(kit.DefaultEnvironment, true)
+	config, _ := env.GetConfiguration(kit.DefaultEnvironment)
 	assert.Nil(t, saveConfiguration(config))
 
 	kittest.GenerateConfig("example.myshopify.io", false)
 	env, _ = kit.LoadEnvironments("config.yml")
-	config, _ = env.GetConfiguration(kit.DefaultEnvironment, true)
+	config, _ = env.GetConfiguration(kit.DefaultEnvironment)
 	assert.NotNil(t, saveConfiguration(config))
 }

cmd/command_arbiter.go

@@ -18,7 +18,6 @@ type (
 		progress           *mpb.Progress
 		verbose            bool
 		force              bool
-		master             string
 		configPath         string
 		allenvs            bool
 		environments       stringArgArray
@@ -28,7 +27,6 @@ type (
 		ignoredFiles       stringArgArray
 		ignores            stringArgArray
 		activeThemeClients []kit.ThemeClient
-		allThemeClients    []kit.ThemeClient
 		manifest           *fileManifest
 	}
 	cobraCmdE     func(*cobra.Command, []string) error
@@ -50,13 +48,12 @@ func newCommandArbiter() *commandArbiter {
 
 func (arbiter *commandArbiter) generateManifest() error {
 	var err error
-	arbiter.manifest, err = newFileManifest(filepath.Dir(arbiter.configPath), arbiter.allThemeClients)
+	arbiter.manifest, err = newFileManifest(filepath.Dir(arbiter.configPath), arbiter.activeThemeClients)
 	return err
 }
 
 func (arbiter *commandArbiter) generateThemeClients(cmd *cobra.Command, args []string) error {
 	arbiter.activeThemeClients = []kit.ThemeClient{}
-	arbiter.allThemeClients = []kit.ThemeClient{}
 	configEnvs, err := kit.LoadEnvironments(arbiter.configPath)
 
 	if err != nil && os.IsNotExist(err) {
@@ -66,8 +63,11 @@ func (arbiter *commandArbiter) generateThemeClients(cmd *cobra.Command, args []s
 	}
 
 	for env := range configEnvs {
-		isActive := arbiter.shouldUseEnvironment(env)
-		config, err := configEnvs.GetConfiguration(env, isActive)
+		if !arbiter.shouldUseEnvironment(env) {
+			continue
+		}
+
+		config, err := configEnvs.GetConfiguration(env)
 		if err != nil {
 			continue
 		}
@@ -88,10 +88,7 @@ func (arbiter *commandArbiter) generateThemeClients(cmd *cobra.Command, args []s
 			return err
 		}
 
-		if isActive {
-			arbiter.activeThemeClients = append(arbiter.activeThemeClients, client)
-		}
-		arbiter.allThemeClients = append(arbiter.allThemeClients, client)
+		arbiter.activeThemeClients = append(arbiter.activeThemeClients, client)
 	}
 
 	if len(arbiter.activeThemeClients) == 0 {
@@ -185,7 +182,7 @@ func (arbiter *commandArbiter) preflightCheck(actions map[string]assetAction, de
 	}
 
 	for _, client := range arbiter.activeThemeClients {
-		diff := arbiter.manifest.Diff(actions, client.Config.Environment, arbiter.master)
+		diff := arbiter.manifest.Diff(actions, client.Config.Environment)
 		if diff.Any(destructive) {
 			return diff
 		}

cmd/command_arbiter_test.go

@@ -69,8 +69,6 @@ func TestCommandArbiter_GenerateThemeClients(t *testing.T) {
 	arbiter.disableIgnore = true
 	assert.Nil(t, arbiter.generateThemeClients(nil, []string{}))
 	assert.Equal(t, 1, len(arbiter.activeThemeClients))
-	assert.Equal(t, 3, len(arbiter.allThemeClients))
-	assert.Equal(t, 0, len(arbiter.allThemeClients[0].Config.IgnoredFiles))
 
 	arbiter.environments = stringArgArray{[]string{}}
 	assert.Nil(t, arbiter.generateThemeClients(nil, []string{}))

cmd/file_manifest.go

@@ -46,10 +46,6 @@ func newFileManifest(path string, clients []kit.ThemeClient) (*fileManifest, err
 		return nil, err
 	}
 
-	if err := manifest.backfillLocal(); err != nil {
-		return nil, err
-	}
-
 	return manifest, manifest.prune(clients)
 }
 
@@ -86,29 +82,6 @@ func (manifest *fileManifest) generateRemote(clients []kit.ThemeClient) error {
 	return requestGroup.Wait()
 }
 
-func (manifest *fileManifest) backfillLocal() (err error) {
-	batch := manifest.store.Batch()
-
-	for filename, remoteEnvs := range manifest.remote {
-		if localEnvs, found := manifest.local[filename]; found {
-			for env, version := range remoteEnvs {
-				if _, hasLocal := localEnvs[env]; !hasLocal {
-					if err = batch.Write(filename, env, version); err != nil {
-						return err
-					}
-				}
-			}
-		}
-	}
-
-	if err = batch.Commit(); err != nil {
-		return err
-	}
-
-	manifest.local, err = manifest.store.Dump()
-	return err
-}
-
 func (manifest *fileManifest) prune(clients []kit.ThemeClient) error {
 	// prune files that are no longer on disk
 	for filename := range manifest.local {
@@ -145,26 +118,26 @@ func parseTime(t string) time.Time {
 	return parsed
 }
 
-func (manifest *fileManifest) diffDates(filename, dstEnv, srcEnv string) (local, remote time.Time) {
+func (manifest *fileManifest) diffDates(filename, environment string) (local, remote time.Time) {
 	manifest.mutex.Lock()
 	defer manifest.mutex.Unlock()
 
 	if _, ok := manifest.local[filename]; ok {
-		local = parseTime(strings.Split(manifest.local[filename][srcEnv], versionSeparator)[0])
+		local = parseTime(strings.Split(manifest.local[filename][environment], versionSeparator)[0])
 	}
 	if _, ok := manifest.remote[filename]; ok {
-		remote = parseTime(strings.Split(manifest.remote[filename][dstEnv], versionSeparator)[0])
+		remote = parseTime(manifest.remote[filename][environment])
 	}
 	return local, remote
 }
 
 func (manifest *fileManifest) NeedsDownloading(filename, environment string) bool {
-	localTime, remoteTime := manifest.diffDates(filename, environment, environment)
+	localTime, remoteTime := manifest.diffDates(filename, environment)
 	return localTime.Before(remoteTime) || localTime.IsZero()
 }
 
 func (manifest *fileManifest) ShouldUpload(asset kit.Asset, environment string) bool {
-	localTime, remoteTime := manifest.diffDates(asset.Key, environment, environment)
+	localTime, remoteTime := manifest.diffDates(asset.Key, environment)
 
 	manifest.mutex.Lock()
 	defer manifest.mutex.Unlock()
@@ -174,7 +147,7 @@ func (manifest *fileManifest) ShouldUpload(asset kit.Asset, environment string)
 }
 
 func (manifest *fileManifest) ShouldRemove(filename, environment string) bool {
-	localTime, remoteTime := manifest.diffDates(filename, environment, environment)
+	localTime, remoteTime := manifest.diffDates(filename, environment)
 	return remoteTime.Before(localTime) || remoteTime.Equal(localTime) || localTime.IsZero()
 }
 
@@ -225,10 +198,10 @@ func fmtTime(t time.Time) string {
 	return "[" + t.Format("Jan 2 3:04PM 2006") + "]"
 }
 
-func (manifest *fileManifest) Diff(actions map[string]assetAction, dstEnv, srcEnv string) *themeDiff {
+func (manifest *fileManifest) Diff(actions map[string]assetAction, environment string) *themeDiff {
 	diff := newDiff()
 	for filename, action := range actions {
-		local, remote := manifest.diffDates(filename, dstEnv, srcEnv)
+		local, remote := manifest.diffDates(filename, environment)
 		if !local.IsZero() && remote.IsZero() {
 			diff.Removed = append(diff.Removed, red(filename+" "+fmtTime(local)))
 		}
@@ -243,31 +216,19 @@ func (manifest *fileManifest) Diff(actions map[string]assetAction, dstEnv, srcEn
 }
 
 func (manifest *fileManifest) Set(filename, environment, version, checksum string) error {
-	var err error
 	manifest.mutex.Lock()
 	defer manifest.mutex.Unlock()
 
 	if _, ok := manifest.remote[filename]; !ok {
 		manifest.remote[filename] = make(map[string]string)
 	}
-	manifest.remote[filename][environment] = strings.Join([]string{version, checksum}, versionSeparator)
+	manifest.remote[filename][environment] = version
 
-	batch := manifest.store.Batch()
-	for env, version := range manifest.remote[filename] {
-		currentVersion, _ := manifest.store.Read(filename, environment)
-		if currentVersion == "" || env == environment {
-			if err = batch.Write(filename, env, version); err != nil {
-				return err
-			}
-		}
+	versionWithChecksum := version
+	if checksum != "" {
+		versionWithChecksum = strings.Join([]string{version, checksum}, versionSeparator)
 	}
-
-	if err = batch.Commit(); err != nil {
-		return err
-	}
-
-	manifest.local, err = manifest.store.Dump()
-	return err
+	return manifest.store.Write(filename, environment, versionWithChecksum)
 }
 
 func (manifest *fileManifest) Delete(filename, environment string) error {

cmd/file_manifest_test.go

@@ -40,32 +40,6 @@ func TestFileManifest_GenerateRemote(t *testing.T) {
 	}
 }
 
-func TestFileManifest_backfillLocal(t *testing.T) {
-	server := kittest.NewTestServer()
-	defer server.Close()
-	assert.Nil(t, kittest.GenerateConfig(server.URL, true))
-	defer kittest.Cleanup()
-	defer resetArbiter()
-
-	file, env1, env2, now, then := "asset.js", "development", "production", "2017-07-06T02:04:21-11:00", "2012-07-06T02:04:21-11:00"
-	store, _ := ystore.New(storeName)
-	defer store.Drop()
-	store.Write(file, env1, then)
-	store.Write(file, "other", then)
-	manifest := &fileManifest{
-		store:  store,
-		local:  map[string]map[string]string{file: {env1: then, "other": then}},
-		remote: map[string]map[string]string{file: {env1: now, env2: now}},
-	}
-
-	_, ok := manifest.local[file][env2]
-	assert.False(t, ok)
-	assert.Nil(t, manifest.backfillLocal())
-	assert.Equal(t, then, manifest.local[file][env1])
-	assert.Equal(t, then, manifest.local[file]["other"])
-	assert.Equal(t, now, manifest.local[file][env2])
-}
-
 func TestFileManifest_Prune(t *testing.T) {
 	server := kittest.NewTestServer()
 	defer server.Close()
@@ -110,13 +84,13 @@ func TestFileManifest_DiffDates(t *testing.T) {
 		remote: make(map[string]map[string]string),
 	}
 
-	local, remote := manifest.diffDates("asset.js", "production", "development")
+	local, remote := manifest.diffDates("asset.js", "production")
 	assert.True(t, local.IsZero())
 	assert.True(t, remote.IsZero())
 
-	manifest.local["asset.js"] = map[string]string{"development": "2012-07-06T02:04:21-11:00"}
+	manifest.local["asset.js"] = map[string]string{"production": "2012-07-06T02:04:21-11:00"}
 	manifest.remote["asset.js"] = map[string]string{"production": "2012-07-06T02:04:21-11:00"}
-	local, remote = manifest.diffDates("asset.js", "production", "development")
+	local, remote = manifest.diffDates("asset.js", "production")
 	assert.False(t, local.IsZero())
 	assert.False(t, remote.IsZero())
 }
@@ -192,15 +166,15 @@ func TestFileManifest_FetchableFiles(t *testing.T) {
 }
 
 func TestFileManifest_Diff(t *testing.T) {
-	dstenv, srcenv, now, then := "production", "development", "2017-07-06T02:04:21-11:00", "2012-07-06T02:04:21-11:00"
+	env, now, then := "production", "2017-07-06T02:04:21-11:00", "2012-07-06T02:04:21-11:00"
 	manifest := &fileManifest{
 		local: map[string]map[string]string{
-			"asset1.js": {srcenv: now},
-			"asset2.js": {srcenv: then},
+			"asset1.js": {env: now},
+			"asset2.js": {env: then},
 		},
 		remote: map[string]map[string]string{
-			"asset2.js": {dstenv: now},
-			"asset3.js": {dstenv: then},
+			"asset2.js": {env: now},
+			"asset3.js": {env: then},
 		},
 	}
 
@@ -210,7 +184,7 @@ func TestFileManifest_Diff(t *testing.T) {
 		"asset3.js": {event: kit.Remove},
 	}
 
-	diff := manifest.Diff(actions, dstenv, srcenv)
+	diff := manifest.Diff(actions, env)
 	assert.Equal(t, 1, len(diff.Created))
 	assert.Equal(t, 1, len(diff.Updated))
 	assert.Equal(t, 1, len(diff.Removed))

cmd/theme.go

@@ -82,11 +82,6 @@ func init() {
 	replaceCmd.Flags().BoolVarP(&arbiter.force, "force", "f", false, "disable version checking and force all changes")
 	uploadCmd.Flags().BoolVarP(&arbiter.force, "force", "f", false, "disable version checking and force all changes")
 
-	watchCmd.Flags().StringVarP(&arbiter.master, "master", "m", kit.DefaultEnvironment, "The destination from which all changes will be applied")
-	removeCmd.Flags().StringVarP(&arbiter.master, "master", "m", kit.DefaultEnvironment, "The destination from which all changes will be applied")
-	replaceCmd.Flags().StringVarP(&arbiter.master, "master", "m", kit.DefaultEnvironment, "The destination from which all changes will be applied")
-	uploadCmd.Flags().StringVarP(&arbiter.master, "master", "m", kit.DefaultEnvironment, "The destination from which all changes will be applied")
-
 	bootstrapCmd.Flags().StringVar(&bootstrapVersion, "version", latestRelease, "version of Shopify Timber to use")
 	bootstrapCmd.Flags().StringVar(&bootstrapPrefix, "prefix", "", "prefix to the Timber theme being created")
 	bootstrapCmd.Flags().StringVar(&bootstrapURL, "url", "", "a url to pull a project theme zip file from.")

kit/configuration.go

@@ -59,15 +59,13 @@ func SetFlagConfig(config Configuration) {
 // formatted configuration along with any validation errors. The config precedence
 // is flags, environment variables, then the config file.
 func NewConfiguration() (*Configuration, error) {
-	return (&Configuration{}).compile(true)
+	return (&Configuration{}).compile()
 }
 
-func (conf *Configuration) compile(active bool) (*Configuration, error) {
+func (conf *Configuration) compile() (*Configuration, error) {
 	newConfig := &Configuration{}
-	if active {
-		mergo.Merge(newConfig, &flagConfig)
-		mergo.Merge(newConfig, &environmentConfig)
-	}
+	mergo.Merge(newConfig, &flagConfig)
+	mergo.Merge(newConfig, &environmentConfig)
 	mergo.Merge(newConfig, conf)
 	mergo.Merge(newConfig, &defaultConfig)
 	return newConfig, newConfig.Validate()

kit/configuration_test.go

@@ -56,20 +56,16 @@ func TestConfiguration_Precedence(t *testing.T) {
 	defer resetConfig()
 
 	config := &Configuration{Password: "file"}
-	config, _ = config.compile(true)
+	config, _ = config.compile()
 	assert.Equal(t, "file", config.Password)
 
 	environmentConfig = Configuration{Password: "environment"}
-	config, _ = config.compile(true)
+	config, _ = config.compile()
 	assert.Equal(t, "environment", config.Password)
 
 	flagConfig = Configuration{Password: "flag"}
-	config, _ = config.compile(true)
+	config, _ = config.compile()
 	assert.Equal(t, "flag", config.Password)
-
-	config = &Configuration{Password: "file"}
-	config, _ = config.compile(false)
-	assert.Equal(t, "file", config.Password)
 }
 
 func TestConfiguration_Validate(t *testing.T) {

kit/environments.go

@@ -66,7 +66,7 @@ func (e Environments) SetConfiguration(environmentName string, conf *Configurati
 // active parameter indicates if the configuration should take configuration values from
 // environment and flags. This is considered active because passive configurations only
 // take configuration from the config file and defaults.
-func (e Environments) GetConfiguration(environmentName string, active bool) (*Configuration, error) {
+func (e Environments) GetConfiguration(environmentName string) (*Configuration, error) {
 	conf, exists := e[environmentName]
 	if !exists {
 		return conf, fmt.Errorf("%s does not exist in this environments list", environmentName)
@@ -78,7 +78,7 @@ Please see %s for examples`,
 			blue("http://shopify.github.io/themekit/configuration/#config-file"))
 	}
 	conf.Environment = environmentName
-	return conf.compile(active)
+	return conf.compile()
 }
 
 // Save will write out the environment to a file.

kit/environments_test.go

@@ -64,12 +64,12 @@ func TestEnvironments_GetConfiguration(t *testing.T) {
 	kittest.GenerateConfig("example.myshopify.io", true)
 	envs, err := LoadEnvironments("config.yml")
 	assert.Nil(t, err)
-	_, err = envs.GetConfiguration("development", true)
+	_, err = envs.GetConfiguration("development")
 	assert.Nil(t, err)
-	_, err = envs.GetConfiguration("nope", true)
+	_, err = envs.GetConfiguration("nope")
 	assert.NotNil(t, err)
 	envs["test"] = nil
-	_, err = envs.GetConfiguration("test", true)
+	_, err = envs.GetConfiguration("test")
 	assert.NotNil(t, err)
 }
 

kit/file_watcher_test.go

@@ -47,7 +47,7 @@ func TestFileWatcher_WatchSymlinkDirectory(t *testing.T) {
 		Password:  "abc123",
 		Domain:    "test.myshopify.com",
 		Directory: kittest.SymlinkProjectPath,
-	}).compile(true)
+	}).compile()
 	assert.Nil(t, err)
 	println(config.Directory)