Repository: digitalocean/doctl
Pull request #1286 (2022-10-18 15:54:01)

apps: Nest dev-config settings under 'dev'

The `flagName` method that is used when [binding flags to viper](https://github.com/digitalocean/doctl/blob/de1918ad1ee4b1e523c335fa9254e85d415d18aa/commands/doit.go#L221-L230) only supports  nesting a setting three levels deep.

https://github.com/digitalocean/doctl/blob/de1918ad1ee4b1e523c335fa9254e85d415d18aa/commands/doit.go#L287-L292

This causes issues for some deeply nested commands. This is particularly problematic for the `doctl apps dev config set/unset` commands which contain `--dev-config` flag. The resulting setting for it is `config.set.dev-config` which conflicts with the top level `config` setting that should be a string. 

When Viper is init-ed, there is a race to register the setting. Sometimes, it results in the config file containing:

```
config:
  set:
    dev-config: ""
```

Others, it has the appropriate:

    config: /home/<user>/.config/doctl/config.yaml

This breaks things in bad ways causing `doctl auth init` to fail unpredictably. 

This PR forces the `dev-config` setting to be nested more deeply, i.e. `dev.config.set.dev-config`. I don't entirely love this solution, but a more general one would be a breaking change to the configuration file format For example, the settings for DOKS are like `cluster.create.region` rather than `kubernetes.cluster.create.region`. I think we should prioritize getting this specific problem fixed over a broader solution.

Fixes: https://github.com/digitalocean/doctl/issues/1281, https://github.com/digitalocean/doctl/issues/1280

andrewsomething: Pushed an additional commit to not nest deeper if it is already under `dev.`, e.g. if the `app dev build` command gets a `dev-config` flag.

Patch:
diff --git a/commands/apps_dev.go b/commands/apps_dev.go
index 133fcdca..d3b51f57 100644
--- a/commands/apps_dev.go
+++ b/commands/apps_dev.go
@@ -36,7 +36,8 @@ import (
 
 const (
 	// AppsDevDefaultEnvFile is the default env file path.
-	AppsDevDefaultEnvFile = ".env"
+	AppsDevDefaultEnvFile     = ".env"
+	appDevConfigFileNamespace = "dev"
 )
 
 // AppsDev creates the apps dev command subtree.
@@ -64,7 +65,7 @@ func AppsDev() *Command {
 		"Build an app component",
 		heredoc.Docf(`
 			[BETA] Build an app component locally.
-			
+
 			  The component name is optional unless running non-interactively.
 
 			  All command line flags as optional. You may specify flags to be applied to the current build
@@ -220,7 +221,7 @@ func RunAppsDevBuild(c *CmdConfig) error {
 
 			{{warning (print crossmark " functions builds are coming soon!")}}
 			  please use {{highlight "doctl serverless deploy"}} to build functions in the meantime.
-		
+
 		`), nil)
 		return fmt.Errorf("not supported")
 	}
@@ -438,10 +439,10 @@ func RunAppsDevBuild(c *CmdConfig) error {
 		tmpl := `
 				{{success checkmark}} successfully built {{success .component}} in {{highlight (duration .dur)}}
 				{{success checkmark}} created container image {{success .img}}
-				
+
 				{{pointerRight}} push your image to a container registry using {{highlight "docker push"}}
 				{{pointerRight}} or run it locally using {{highlight "docker run"}}; for example:
-				  
+
 				   {{muted promptPrefix}} {{highlight (printf "docker run %s--rm %s%s" .port_env .port_arg .img)}}`
 
 		if _, ok := componentSpec.(godo.AppRoutableComponentSpec); ok {
@@ -481,7 +482,10 @@ func fileExists(path ...string) bool {
 }
 
 func appDevWorkspace(cmdConfig *CmdConfig) (*workspace.AppDev, error) {
-	devConfigFilePath, err := cmdConfig.Doit.GetString(cmdConfig.NS, doctl.ArgAppDevConfig)
+	// The setting is nested under the "dev" namespace, i.e. dev.config.set.dev-config
+	// This is needed to prevent a conflict with the base config setting.
+	ns := fmt.Sprintf("%s.%s", appDevConfigFileNamespace, cmdConfig.NS)
+	devConfigFilePath, err := cmdConfig.Doit.GetString(ns, doctl.ArgAppDevConfig)
 	if err != nil {
 		return nil, err
 	}
@@ -602,7 +606,7 @@ func appsDevBuildSpecRequired(ws *workspace.AppDev, appsService do.AppsService)
 	template.Print(heredoc.Doc(`
 		{{error (print crossmark " no app spec found.")}}
 		  an app spec is required to start a local build. make sure doctl is run in the correct directory where your app code is.
-		
+
 		`,
 	), nil)
 
diff --git a/commands/doit.go b/commands/doit.go
index ba587a84..cfcdce78 100644
--- a/commands/doit.go
+++ b/commands/doit.go
@@ -220,6 +220,14 @@ func requiredOpt() flagOpt {
 // AddStringFlag adds a string flag to a command.
 func AddStringFlag(cmd *Command, name, shorthand, dflt, desc string, opts ...flagOpt) {
 	fn := flagName(cmd, name)
+	// flagName only supports nesting three levels deep. We need to force the
+	// app dev config set/unset --dev-config flag to be nested deeper.
+	// i.e dev.config.set.dev-config over config.set.dev-config
+	// This prevents a conflict with the base config setting.
+	if name == doctl.ArgAppDevConfig && !strings.HasPrefix(fn, appDevConfigFileNamespace+".") {
+		fn = fmt.Sprintf("%s.%s", appDevConfigFileNamespace, fn)
+	}
+
 	cmd.Flags().StringP(name, shorthand, dflt, desc)
 
 	for _, o := range opts {


Test patch:
diff --git a/commands/auth_test.go b/commands/auth_test.go
index 2fc85438..f093fe9a 100644
--- a/commands/auth_test.go
+++ b/commands/auth_test.go
@@ -14,9 +14,11 @@ limitations under the License.
 package commands
 
 import (
+	"bufio"
 	"bytes"
 	"io"
 	"io/ioutil"
+	"path/filepath"
 	"testing"
 
 	"errors"
@@ -25,6 +27,7 @@ import (
 	"github.com/digitalocean/doctl/do"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
+	yaml "gopkg.in/yaml.v2"
 )
 
 func TestAuthCommand(t *testing.T) {
@@ -54,6 +57,50 @@ func TestAuthInit(t *testing.T) {
 	})
 }
 
+func TestAuthInitConfig(t *testing.T) {
+	cfw := cfgFileWriter
+	viper.Set(doctl.ArgAccessToken, nil)
+	defer func() {
+		cfgFileWriter = cfw
+	}()
+
+	retrieveUserTokenFunc := func() (string, error) {
+		return "valid-token", nil
+	}
+
+	var buf bytes.Buffer
+	cfgFileWriter = func() (io.WriteCloser, error) {
+		return &nopWriteCloser{
+			Writer: bufio.NewWriter(&buf),
+		}, nil
+	}
+
+	withTestClient(t, func(config *CmdConfig, tm *tcMocks) {
+		tm.account.EXPECT().Get().Return(&do.Account{}, nil)
+
+		err := RunAuthInit(retrieveUserTokenFunc)(config)
+		assert.NoError(t, err)
+
+		var configFile testConfig
+		err = yaml.Unmarshal(buf.Bytes(), &configFile)
+		assert.NoError(t, err)
+		defaultCfgFile := filepath.Join(defaultConfigHome(), defaultConfigName)
+		assert.Equal(t, configFile["config"], defaultCfgFile, "unexpected setting for 'config'")
+
+		// Ensure that the dev.config.set.dev-config setting is correct to prevent
+		// a conflict with the base config setting.
+		devConfig := configFile["dev"]
+		devConfigSetting := devConfig.(map[interface{}]interface{})["config"]
+		expectedConfigSetting := map[interface{}]interface{}(
+			map[interface{}]interface{}{
+				"set":   map[interface{}]interface{}{"dev-config": ""},
+				"unset": map[interface{}]interface{}{"dev-config": ""},
+			},
+		)
+		assert.Equal(t, devConfigSetting, expectedConfigSetting, "unexpected setting for 'dev.config'")
+	})
+}
+
 func TestAuthInitWithProvidedToken(t *testing.T) {
 	cfw := cfgFileWriter
 	viper.Set(doctl.ArgAccessToken, "valid-token")
@@ -141,6 +188,8 @@ func Test_displayAuthContexts(t *testing.T) {
 	}
 }
 
+type testConfig map[string]interface{}
+
 type nopWriteCloser struct {
 	io.Writer
 }