Prevent inappropriate rewrite of global_config.yaml, fixes #1882, fixes

#2014 (#2040)

* Attempt to make global config mgt more robust for #1882
* Make primary url first in list, fixes #2014
* Write DdevGlobalConfig instead of GlobalConfig{} when writing new config file
* Fix problem with wordpress site where global overwrite happened
* Refactor GetCAROOT() into globalconfig

cmd/ddev/cmd/a.go

@@ -0,0 +1,18 @@
+package cmd
+
+import (
+	"github.com/drud/ddev/pkg/globalconfig"
+	"github.com/drud/ddev/pkg/util"
+)
+
+// This file is a.go because global config must be loaded before anybody else
+// runs their init(), as they might overwrite global_config.yaml with
+// uninitialized data
+
+func init() {
+	err := globalconfig.ReadGlobalConfig()
+	if err != nil {
+		util.Failed("unable to read global config: %v", err)
+	}
+	globalconfig.GetCAROOT()
+}

cmd/ddev/cmd/config-global.go

@@ -30,10 +30,12 @@ func handleGlobalConfig(cmd *cobra.Command, args []string) {
 		util.Failed("Unable to read global config file: %v", err)
 	}
 
+	dirty := false
 	if cmd.Flag("instrumentation-opt-in").Changed {
 		globalconfig.DdevGlobalConfig.InstrumentationOptIn = instrumentationOptIn
 		// Make sure that they don't get prompted again right after they opted out.
-		globalconfig.DdevGlobalConfig.LastUsedVersion = version.VERSION
+		globalconfig.DdevGlobalConfig.LastStartedVersion = version.VERSION
+		dirty = true
 	}
 	if cmd.Flag("omit-containers").Changed {
 		omitContainers = strings.Replace(omitContainers, " ", "", -1)
@@ -42,27 +44,31 @@ func handleGlobalConfig(cmd *cobra.Command, args []string) {
 		} else {
 			globalconfig.DdevGlobalConfig.OmitContainers = strings.Split(omitContainers, ",")
 		}
+		dirty = true
 	}
 	if cmd.Flag("router-bind-all-interfaces").Changed {
 		globalconfig.DdevGlobalConfig.RouterBindAllInterfaces, _ = cmd.Flags().GetBool("router-bind-all-interfaces")
-	}
-	err = globalconfig.ValidateGlobalConfig()
-	if err != nil {
-		util.Failed("Invalid configuration in %s: %v", globalconfig.GetGlobalConfigPath(), err)
-	}
-	err = globalconfig.WriteGlobalConfig(globalconfig.DdevGlobalConfig)
-	if err != nil {
-		util.Failed("Failed to write global config: %v", err)
+		dirty = true
 	}
 
+	if dirty {
+		err = globalconfig.ValidateGlobalConfig()
+		if err != nil {
+			util.Failed("Invalid configuration in %s: %v", globalconfig.GetGlobalConfigPath(), err)
+		}
+		err = globalconfig.WriteGlobalConfig(globalconfig.DdevGlobalConfig)
+		if err != nil {
+			util.Failed("Failed to write global config: %v", err)
+		}
+	}
 	util.Success("Global configuration:")
 	output.UserOut.Printf("instrumentation-opt-in=%v", globalconfig.DdevGlobalConfig.InstrumentationOptIn)
 	output.UserOut.Printf("omit-containers=[%s]", strings.Join(globalconfig.DdevGlobalConfig.OmitContainers, ","))
 	output.UserOut.Printf("router-bind-all-interfaces=%v", globalconfig.DdevGlobalConfig.RouterBindAllInterfaces)
 }
 
 func init() {
-	configGlobalCommand.Flags().StringVarP(&omitContainers, "omit-containers", "", "", "omit-containers=dba,ddev-ssh-agent")
+	configGlobalCommand.Flags().StringVarP(&omitContainers, "omit-containers", "", "", "For example, --omit-containers=dba,ddev-ssh-agent")
 	configGlobalCommand.Flags().BoolVarP(&instrumentationOptIn, "instrumentation-opt-in", "", false, "instrmentation-opt-in=true")
 	configGlobalCommand.Flags().Bool("router-bind-all-interfaces", false, "router-bind-all-interfaces=true")
 

cmd/ddev/cmd/config-global_test.go

@@ -17,6 +17,7 @@ import (
 func TestCmdGlobalConfig(t *testing.T) {
 	assert := asrt.New(t)
 
+	backupConfig := globalconfig.DdevGlobalConfig
 	// Start with no config file
 	configFile := globalconfig.GetGlobalConfigPath()
 	if fileutil.FileExists(configFile) {
@@ -26,9 +27,19 @@ func TestCmdGlobalConfig(t *testing.T) {
 	// We need to make sure that the (corrupted, bogus) global config file is removed
 	// and then read (empty)
 	// nolint: errcheck
-	defer globalconfig.ReadGlobalConfig()
-	// nolint: errcheck
-	defer os.Remove(configFile)
+	defer func() {
+		globalconfig.DdevGlobalConfig = backupConfig
+		globalconfig.DdevGlobalConfig.OmitContainers = nil
+
+		err := os.Remove(configFile)
+		if err != nil {
+			t.Logf("Unable to remove %v: %v", configFile, err)
+		}
+		err = globalconfig.ReadGlobalConfig()
+		if err != nil {
+			t.Logf("Unable to ReadGlobalConfig: %v", err)
+		}
+	}()
 
 	// Look at initial config
 	args := []string{"config", "global"}
@@ -51,6 +62,7 @@ func TestCmdGlobalConfig(t *testing.T) {
 
 	// Even though the global config is going to be deleted, make sure it's sane before leaving
 	args = []string{"config", "global", "--omit-containers", ""}
+	globalconfig.DdevGlobalConfig.OmitContainers = nil
 	_, err = exec.RunCommand(DdevBin, args)
 	assert.NoError(err)
 }

cmd/ddev/cmd/list_test.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"bufio"
+	"github.com/drud/ddev/pkg/globalconfig"
 	"github.com/stretchr/testify/require"
 	"runtime"
 	"strings"
@@ -43,7 +44,7 @@ func TestCmdList(t *testing.T) {
 		// Look for standard items in the regular ddev list output
 		assert.Contains(string(out), v.Name)
 		testURL := app.GetHTTPSURL()
-		if ddevapp.GetCAROOT() == "" {
+		if globalconfig.GetCAROOT() == "" {
 			testURL = app.GetHTTPURL()
 		}
 		assert.Contains(string(out), testURL)

cmd/ddev/cmd/restart.go

@@ -1,7 +1,7 @@
 package cmd
 
 import (
-	"github.com/drud/ddev/pkg/ddevapp"
+	"github.com/drud/ddev/pkg/globalconfig"
 	"strings"
 
 	"github.com/drud/ddev/pkg/dockerutil"
@@ -41,7 +41,7 @@ var RestartCmd = &cobra.Command{
 
 			util.Success("Restarted %s", app.GetName())
 			httpURLs, urlList, _ := app.GetAllURLs()
-			if ddevapp.GetCAROOT() == "" {
+			if globalconfig.GetCAROOT() == "" {
 				urlList = httpURLs
 			}
 

cmd/ddev/cmd/root.go

@@ -9,6 +9,7 @@ import (
 	"github.com/drud/ddev/pkg/updatecheck"
 	"github.com/drud/ddev/pkg/util"
 	"github.com/drud/ddev/pkg/version"
+	"github.com/rogpeppe/go-internal/semver"
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
@@ -184,12 +185,12 @@ func instrumentationNotSetUpWarning() {
 // from the last saved version. If it is, prompt to request anon ddev usage stats
 // and update the info.
 func checkDdevVersionAndOptInInstrumentation() error {
-	if !output.JSONOutput && version.COMMIT != globalconfig.DdevGlobalConfig.LastUsedVersion && globalconfig.DdevGlobalConfig.InstrumentationOptIn == false && !globalconfig.DdevNoInstrumentation {
+	if !output.JSONOutput && semver.Compare(version.COMMIT, globalconfig.DdevGlobalConfig.LastStartedVersion) > 0 && globalconfig.DdevGlobalConfig.InstrumentationOptIn == false && !globalconfig.DdevNoInstrumentation {
 		allowStats := util.Confirm("It looks like you have a new ddev release.\nMay we send anonymous ddev usage statistics and errors?\nTo know what we will see please take a look at\nhttps://ddev.readthedocs.io/en/stable/users/cli-usage/#opt-in-usage-information\nPermission to beam up?")
 		if allowStats {
 			globalconfig.DdevGlobalConfig.InstrumentationOptIn = true
 		}
-		globalconfig.DdevGlobalConfig.LastUsedVersion = version.VERSION
+		globalconfig.DdevGlobalConfig.LastStartedVersion = version.VERSION
 		err := globalconfig.WriteGlobalConfig(globalconfig.DdevGlobalConfig)
 		if err != nil {
 			return err

cmd/ddev/cmd/start.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"github.com/drud/ddev/pkg/ddevapp"
+	"github.com/drud/ddev/pkg/globalconfig"
 	"strings"
 
 	"github.com/drud/ddev/pkg/dockerutil"
@@ -49,7 +50,7 @@ any directory by running 'ddev start projectname [projectname ...]'`,
 
 			util.Success("Successfully started %s", project.GetName())
 			httpURLs, urlList, _ := project.GetAllURLs()
-			if ddevapp.GetCAROOT() == "" {
+			if globalconfig.GetCAROOT() == "" {
 				urlList = httpURLs
 			}
 			util.Success("Project can be reached at %s", strings.Join(urlList, " "))

pkg/ddevapp/config.go

@@ -458,8 +458,6 @@ func (app *DdevApp) GetHostnames() []string {
 	// The value is useless, so just use the int 1 for assignment.
 	nameListMap := make(map[string]int)
 
-	nameListMap[app.GetHostname()] = 1
-
 	for _, name := range app.AdditionalHostnames {
 		nameListMap[name+"."+app.ProjectTLD] = 1
 	}
@@ -468,12 +466,18 @@ func (app *DdevApp) GetHostnames() []string {
 		nameListMap[name] = 1
 	}
 
+	// Make sure the primary hostname didn't accidentally get added, it will be prepended
+	delete(nameListMap, app.GetHostname())
+
 	// Now walk the map and extract the keys into an array.
 	nameListArray := make([]string, 0, len(nameListMap))
 	for k := range nameListMap {
 		nameListArray = append(nameListArray, k)
 	}
 	sort.Strings(nameListArray)
+	// We want the primary hostname to be first in the list.
+	nameListArray = append([]string{app.GetHostname()}, nameListArray...)
+
 	return nameListArray
 }
 

pkg/ddevapp/ddevapp.go

@@ -759,7 +759,7 @@ func (app *DdevApp) Start() error {
 	// Warn the user if there is any custom configuration in use.
 	app.CheckCustomConfig()
 
-	caRoot := GetCAROOT()
+	caRoot := globalconfig.GetCAROOT()
 	if caRoot == "" {
 		util.Warning("mkcert may not be properly installed, we suggest installing it for trusted https support, `brew install mkcert nss`, `choco install -y mkcert`, etc. and then `mkcert -install`")
 	}
@@ -1445,8 +1445,10 @@ func (app *DdevApp) GetAllURLs() (httpURLs []string, httpsURLs []string, allURLs
 
 // GetPrimaryURL() returns the primary URL that can be used, https or http
 func (app *DdevApp) GetPrimaryURL() string {
-	httpURLs, urlList, _ := app.GetAllURLs()
-	if GetCAROOT() == "" {
+	httpURLs, httpsURLs, _ := app.GetAllURLs()
+	urlList := httpsURLs
+	// If no mkcert trusted https, use the httpURLs instead
+	if globalconfig.GetCAROOT() == "" {
 		urlList = httpURLs
 	}
 	return urlList[0]

pkg/ddevapp/ddevapp_test.go

@@ -2354,7 +2354,7 @@ func TestGetAllURLs(t *testing.T) {
 	require.NotEmpty(t, webContainer)
 
 	expectedDirectAddress := app.GetWebContainerDirectHTTPSURL()
-	if ddevapp.GetCAROOT() == "" {
+	if globalconfig.GetCAROOT() == "" {
 		expectedDirectAddress = app.GetWebContainerDirectHTTPURL()
 	}
 

pkg/ddevapp/utils.go

@@ -95,7 +95,7 @@ func RenderAppRow(table *uitable.Table, row map[string]interface{}) {
 
 	urls := ""
 	if row["status"] == SiteRunning {
-		if GetCAROOT() != "" {
+		if globalconfig.GetCAROOT() != "" {
 			urls = row["httpsurl"].(string)
 		} else {
 			urls = row["httpurl"].(string)