Repository: onflow/flow-cli
Pull request #363 (2021-08-24 14:28:09)

Refactor config loading
Refactor config loader code so it's cleaner. Add tests for saving config function. Move the check for default paths when editing config to a common function. 

______

For contributor use:

- [x] Targeted PR against `master` branch
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work
- [x] Code follows the [standards mentioned here](https://github.com/onflow/flow-cli/blob/master/CONTRIBUTING.md#styleguides)
- [x] Updated relevant documentation
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Added appropriate labels



Patch:
diff --git a/internal/config/add-account.go b/internal/config/add-account.go
index 56ace8bc..ad225936 100644
--- a/internal/config/add-account.go
+++ b/internal/config/add-account.go
@@ -60,15 +60,6 @@ func addAccount(
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-
-	if !config.IsGlobalPath(globalFlags.ConfigPaths) && len(globalFlags.ConfigPaths) > 1 {
-		return nil, fmt.Errorf("specifying multiple paths to -f is not supported when updating configuration")
-	}
-
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	accountData, flagsProvided, err := flagsToAccountData(addAccountFlags)
 	if err != nil {
 		return nil, err
@@ -97,7 +88,7 @@ func addAccount(
 
 	state.Accounts().AddOrUpdate(&acc)
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/add-contract.go b/internal/config/add-contract.go
index d68fe174..71b835aa 100644
--- a/internal/config/add-contract.go
+++ b/internal/config/add-contract.go
@@ -59,15 +59,6 @@ func addContract(
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-
-	if !config.IsGlobalPath(globalFlags.ConfigPaths) && len(globalFlags.ConfigPaths) > 1 {
-		return nil, fmt.Errorf("specifying multiple paths to -f is not supported when updating configuration")
-	}
-
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	contractData, flagsProvided, err := flagsToContractData(addContractFlags)
 	if err != nil {
 		return nil, err
@@ -89,7 +80,7 @@ func addContract(
 		state.Contracts().AddOrUpdate(contract.Name, contract)
 	}
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/add-deployment.go b/internal/config/add-deployment.go
index 77fe47be..1be27bc9 100644
--- a/internal/config/add-deployment.go
+++ b/internal/config/add-deployment.go
@@ -57,15 +57,6 @@ func addDeployment(
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-
-	if !config.IsGlobalPath(globalFlags.ConfigPaths) && len(globalFlags.ConfigPaths) > 1 {
-		return nil, fmt.Errorf("specifying multiple paths to -f is not supported when updating configuration")
-	}
-
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	deployData, flagsProvided, err := flagsToDeploymentData(addDeploymentFlags)
 	if err != nil {
 		return nil, err
@@ -83,7 +74,7 @@ func addDeployment(
 
 	state.Deployments().AddOrUpdate(deployment)
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/add-network.go b/internal/config/add-network.go
index a5485591..93265d4e 100644
--- a/internal/config/add-network.go
+++ b/internal/config/add-network.go
@@ -57,15 +57,6 @@ func addNetwork(
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-
-	if !config.IsGlobalPath(globalFlags.ConfigPaths) && len(globalFlags.ConfigPaths) > 1 {
-		return nil, fmt.Errorf("specifying multiple paths to -f is not supported when updating configuration")
-	}
-
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	networkData, flagsProvided, err := flagsToNetworkData(addNetworkFlags)
 	if err != nil {
 		return nil, err
@@ -78,7 +69,7 @@ func addNetwork(
 	network := config.StringToNetwork(networkData["name"], networkData["host"])
 	state.Networks().AddOrUpdate(network.Name, network)
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/remove-account.go b/internal/config/remove-account.go
index 75ba180c..85d7de0b 100644
--- a/internal/config/remove-account.go
+++ b/internal/config/remove-account.go
@@ -19,12 +19,10 @@
 package config
 
 import (
-	"github.com/onflow/flow-cli/pkg/flowkit"
-	"github.com/onflow/flow-cli/pkg/flowkit/config"
-
 	"github.com/spf13/cobra"
 
 	"github.com/onflow/flow-cli/internal/command"
+	"github.com/onflow/flow-cli/pkg/flowkit"
 	"github.com/onflow/flow-cli/pkg/flowkit/output"
 	"github.com/onflow/flow-cli/pkg/flowkit/services"
 )
@@ -47,14 +45,10 @@ var RemoveAccountCommand = &command.Command{
 func removeAccount(
 	args []string,
 	_ flowkit.ReaderWriter,
-	_ command.GlobalFlags,
+	globalFlags command.GlobalFlags,
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	name := ""
 	if len(args) == 1 {
 		name = args[0]
@@ -67,7 +61,7 @@ func removeAccount(
 		return nil, err
 	}
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/remove-contract.go b/internal/config/remove-contract.go
index 3b71245c..0a0efa41 100644
--- a/internal/config/remove-contract.go
+++ b/internal/config/remove-contract.go
@@ -19,12 +19,10 @@
 package config
 
 import (
-	"github.com/onflow/flow-cli/pkg/flowkit"
-	"github.com/onflow/flow-cli/pkg/flowkit/config"
-
 	"github.com/spf13/cobra"
 
 	"github.com/onflow/flow-cli/internal/command"
+	"github.com/onflow/flow-cli/pkg/flowkit"
 	"github.com/onflow/flow-cli/pkg/flowkit/output"
 	"github.com/onflow/flow-cli/pkg/flowkit/services"
 )
@@ -47,14 +45,10 @@ var RemoveContractCommand = &command.Command{
 func removeContract(
 	args []string,
 	_ flowkit.ReaderWriter,
-	_ command.GlobalFlags,
+	globalFlags command.GlobalFlags,
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	name := ""
 	if len(args) == 1 {
 		name = args[0]
@@ -67,7 +61,7 @@ func removeContract(
 		return nil, err
 	}
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/remove-deployment.go b/internal/config/remove-deployment.go
index 47977fa8..2cb7360d 100644
--- a/internal/config/remove-deployment.go
+++ b/internal/config/remove-deployment.go
@@ -23,7 +23,6 @@ import (
 
 	"github.com/onflow/flow-cli/internal/command"
 	"github.com/onflow/flow-cli/pkg/flowkit"
-	"github.com/onflow/flow-cli/pkg/flowkit/config"
 	"github.com/onflow/flow-cli/pkg/flowkit/output"
 	"github.com/onflow/flow-cli/pkg/flowkit/services"
 )
@@ -46,14 +45,10 @@ var RemoveDeploymentCommand = &command.Command{
 func removeDeployment(
 	args []string,
 	_ flowkit.ReaderWriter,
-	_ command.GlobalFlags,
+	globalFlags command.GlobalFlags,
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	account := ""
 	network := ""
 	if len(args) == 2 {
@@ -68,7 +63,7 @@ func removeDeployment(
 		return nil, err
 	}
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/config/remove-network.go b/internal/config/remove-network.go
index 6e8325f3..7452d815 100644
--- a/internal/config/remove-network.go
+++ b/internal/config/remove-network.go
@@ -23,7 +23,6 @@ import (
 
 	"github.com/onflow/flow-cli/internal/command"
 	"github.com/onflow/flow-cli/pkg/flowkit"
-	"github.com/onflow/flow-cli/pkg/flowkit/config"
 	"github.com/onflow/flow-cli/pkg/flowkit/output"
 	"github.com/onflow/flow-cli/pkg/flowkit/services"
 )
@@ -46,14 +45,10 @@ var RemoveNetworkCommand = &command.Command{
 func removeNetwork(
 	args []string,
 	_ flowkit.ReaderWriter,
-	_ command.GlobalFlags,
+	globalFlags command.GlobalFlags,
 	_ *services.Services,
 	state *flowkit.State,
 ) (command.Result, error) {
-	if state == nil {
-		return nil, config.ErrDoesNotExist
-	}
-
 	name := ""
 	if len(args) == 1 {
 		name = args[0]
@@ -66,7 +61,7 @@ func removeNetwork(
 		return nil, err
 	}
 
-	err = state.SaveDefault()
+	err = state.SaveEdited(globalFlags.ConfigPaths)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/flowkit/config/config.go b/pkg/flowkit/config/config.go
index 516e4b38..063b2e9e 100644
--- a/pkg/flowkit/config/config.go
+++ b/pkg/flowkit/config/config.go
@@ -99,7 +99,7 @@ var ErrOutdatedFormat = errors.New("you are using old configuration format")
 
 const DefaultPath = "flow.json"
 
-func IsGlobalPath(paths []string) bool {
+func IsDefaultPath(paths []string) bool {
 	return len(paths) == 2 && paths[0] == GlobalPath() && paths[1] == DefaultPath
 }
 
diff --git a/pkg/flowkit/config/loader.go b/pkg/flowkit/config/loader.go
index 5878ffbe..320e448c 100644
--- a/pkg/flowkit/config/loader.go
+++ b/pkg/flowkit/config/loader.go
@@ -89,6 +89,10 @@ func (l *Loader) Save(conf *Config, path string) error {
 		filepath.Ext(path),
 	)
 
+	if configFormat == nil {
+		return fmt.Errorf("parser not found for format")
+	}
+
 	data, err := configFormat.Serialize(conf)
 	if err != nil {
 		return err
@@ -125,36 +129,35 @@ func (l *Loader) LoadConfig(confPath string) (*Config, error) {
 // If more than one path is specified, their contents are merged
 // together into on configuration object.
 func (l *Loader) Load(paths []string) (*Config, error) {
-	var baseConf *Config
-
-	if IsGlobalPath(paths) {
-		var defaultConfigError error
-		var globalConfigError error
-		//try to load default flow.json
-		baseConf, defaultConfigError = l.LoadConfig(paths[1])
-		if defaultConfigError != nil {
-			//try to load global flow.json
-			baseConf, globalConfigError = l.LoadConfig(paths[0])
-			if globalConfigError != nil {
-				return nil, ErrDoesNotExist
-			}
+	// special case for default configs
+	// try to load local config and only if not found try to load global config
+	if IsDefaultPath(paths) {
+		conf, err := l.LoadConfig(DefaultPath)
+		if err == nil {
+			return l.postprocess(conf)
 		}
-	} else {
-
-		for _, confPath := range paths {
 
-			conf, err := l.LoadConfig(confPath)
-			if err != nil {
-				return nil, err
-			}
-			// if first conf just assign as baseConf
-			if baseConf == nil {
-				baseConf = conf
-				continue
-			}
+		conf, err = l.LoadConfig(GlobalPath())
+		if err != nil {
+			return nil, ErrDoesNotExist
+		} else {
+			return l.postprocess(conf)
+		}
+	}
 
-			l.composeConfig(baseConf, conf)
+	var baseConf *Config
+	for _, confPath := range paths {
+		conf, err := l.LoadConfig(confPath)
+		if err != nil {
+			return nil, err
+		}
+		// if first conf just assign as baseConf
+		if baseConf == nil {
+			baseConf = conf
+			continue
 		}
+
+		l.composeConfig(baseConf, conf)
 	}
 
 	// if no config was loaded - neither local nor global return an error.
@@ -162,12 +165,7 @@ func (l *Loader) Load(paths []string) (*Config, error) {
 		return nil, ErrDoesNotExist
 	}
 
-	baseConf, err := l.postprocess(baseConf)
-	if err != nil {
-		return nil, err
-	}
-
-	return baseConf, nil
+	return l.postprocess(baseConf)
 }
 
 // preprocess does all manipulations to the raw configuration format happens here.
diff --git a/pkg/flowkit/state.go b/pkg/flowkit/state.go
index 1bfb0b94..bbf232b2 100644
--- a/pkg/flowkit/state.go
+++ b/pkg/flowkit/state.go
@@ -67,11 +67,30 @@ func (p *State) ReadFile(source string) ([]byte, error) {
 	return p.readerWriter.ReadFile(source)
 }
 
-// SaveDefault saves configuration to default path.
+// SaveDefault saves to default path.
 func (p *State) SaveDefault() error {
 	return p.Save(config.DefaultPath)
 }
 
+// SaveEdited saves configuration to valid path.
+func (p *State) SaveEdited(paths []string) error {
+	// if paths are not default only allow specifying one config
+	if !config.IsDefaultPath(paths) && len(paths) > 1 {
+		return fmt.Errorf("specifying multiple paths is not supported when updating configuration")
+	}
+	// if default paths and local config doesn't exist don't allow updating global config
+	if config.IsDefaultPath(paths) {
+		_, err := p.confLoader.Load([]string{config.DefaultPath}) // check if default is present
+		if err != nil {
+			return fmt.Errorf("default configuration not found, please initialize it first or specify another configuration file")
+		} else {
+			return p.SaveDefault()
+		}
+	}
+
+	return p.Save(paths[0])
+}
+
 // Save saves the project configuration to the given path.
 func (p *State) Save(path string) error {
 	p.conf.Accounts = accountsToConfig(*p.accounts)


Test patch:
diff --git a/pkg/flowkit/config/loader_test.go b/pkg/flowkit/config/loader_test.go
index 00dfe956..52f234e1 100644
--- a/pkg/flowkit/config/loader_test.go
+++ b/pkg/flowkit/config/loader_test.go
@@ -94,11 +94,11 @@ func Test_AllowMissingLocalJson(t *testing.T) {
 	composer.AddConfigParser(json.NewParser())
 
 	conf, loadErr := composer.Load(config.DefaultPaths())
+	assert.NoError(t, loadErr)
 
 	acc, err := conf.Accounts.ByName("emulator-account")
 	assert.NoError(t, err)
 
-	assert.NoError(t, loadErr)
 	assert.Equal(t, 1, len(conf.Accounts))
 	assert.Equal(t, "0x21c5dfdeb0ff03a7a73ef39788563b62c89adea67bbb21ab95e5f710bd1d40b7",
 		acc.Key.PrivateKey.String(),
diff --git a/pkg/flowkit/state_test.go b/pkg/flowkit/state_test.go
index 9f2c992f..ea55548b 100644
--- a/pkg/flowkit/state_test.go
+++ b/pkg/flowkit/state_test.go
@@ -23,6 +23,8 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/onflow/flow-cli/pkg/flowkit/config/json"
+
 	"github.com/onflow/flow-go-sdk"
 	"github.com/onflow/flow-go-sdk/crypto"
 	"github.com/spf13/afero"
@@ -187,6 +189,7 @@ func generateSimpleProject() State {
 		}},
 	}
 
+	composer.AddConfigParser(json.NewParser())
 	p, err := newProject(&config, composer, af)
 	if err != nil {
 		fmt.Println(err)
@@ -553,3 +556,25 @@ func Test_ChangingState(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, (*pkey).String(), pk.String())
 }
+
+func Test_Saving(t *testing.T) {
+	s := generateSimpleProject()
+
+	err := s.SaveEdited([]string{"a.json", "b.json"})
+	assert.EqualError(t, err, "specifying multiple paths is not supported when updating configuration")
+
+	err = s.SaveEdited([]string{config.GlobalPath(), config.DefaultPath})
+	assert.EqualError(t, err, "default configuration not found, please initialize it first or specify another configuration file")
+
+	err = s.SaveEdited([]string{"a.json"})
+	assert.NoError(t, err)
+
+	_ = afero.WriteFile(af.Fs, config.DefaultPath, []byte(`{
+		"networks": {
+			"foo": "localhost:3000"
+		}
+	}`), 0644)
+
+	err = s.SaveEdited([]string{config.GlobalPath(), config.DefaultPath})
+	assert.NoError(t, err)
+}