Deprecate -endpoint flag

CHANGELOG.md

@@ -14,13 +14,14 @@ All notable changes to `src-cli` are documented in this file.
 ### Added
 
 - Pull missing docker images automatically. [#191](https://github.com/sourcegraph/src-cli/pull/191)
-
 - Searches that result in errors will now display any alerts returned by Sourcegraph, including suggestions for how the search could be corrected. [#221](https://github.com/sourcegraph/src-cli/pull/221)
 
 ### Changed
 
 - The terminal UI has been replaced by the logger-based UI that was previously only visible in verbose-mode (`-v`). [#228](https://github.com/sourcegraph/src-cli/pull/228)
+- Deprecated the `-endpoint` flag. Instead, use the `SRC_ENDPOINT` environment variable. [#235](https://github.com/sourcegraph/src-cli/pull/235)
 
 ### Fixed
 
 ### Removed
+

cmd/src/main.go

@@ -9,6 +9,8 @@ import (
 	"os/user"
 	"path/filepath"
 	"strings"
+
+	"github.com/pkg/errors"
 )
 
 const usageText = `src is a tool that provides access to Sourcegraph instances.
@@ -24,7 +26,6 @@ Environment variables
 
 The options are:
 
-	-endpoint=                       specifies the endpoint to use e.g. "https://sourcegraph.com" (overrides SRC_ENDPOINT if set)
 	-v                               print verbose output
 
 The commands are:
@@ -47,9 +48,11 @@ Use "src [command] -h" for more information about a command.
 `
 
 var (
+	verbose = flag.Bool("v", false, "print verbose output")
+
+	// The following arguments are deprecated which is why they are no longer documented
 	configPath = flag.String("config", "", "")
 	endpoint   = flag.String("endpoint", "", "")
-	verbose    = flag.Bool("v", false, "print verbose output")
 )
 
 // commands contains all registered subcommands.
@@ -76,14 +79,14 @@ func readConfig() (*config, error) {
 	cfgPath := *configPath
 	userSpecified := *configPath != ""
 
-	user, err := user.Current()
+	u, err := user.Current()
 	if err != nil {
 		return nil, err
 	}
 	if !userSpecified {
-		cfgPath = filepath.Join(user.HomeDir, "src-config.json")
+		cfgPath = filepath.Join(u.HomeDir, "src-config.json")
 	} else if strings.HasPrefix(cfgPath, "~/") {
-		cfgPath = filepath.Join(user.HomeDir, cfgPath[2:])
+		cfgPath = filepath.Join(u.HomeDir, cfgPath[2:])
 	}
 	data, err := ioutil.ReadFile(os.ExpandEnv(cfgPath))
 	if err != nil && (!os.IsNotExist(err) || userSpecified) {
@@ -96,23 +99,39 @@ func readConfig() (*config, error) {
 		}
 	}
 
+	envToken := os.Getenv("SRC_ACCESS_TOKEN")
+	envEndpoint := os.Getenv("SRC_ENDPOINT")
+
+	if userSpecified {
+		// If a config file is present, either zero or both environment variables must be present.
+		// We don't want to partially apply environment variables.
+		if envToken == "" && envEndpoint != "" {
+			return nil, errConfigMerge
+		}
+		if envToken != "" && envEndpoint == "" {
+			return nil, errConfigMerge
+		}
+	}
+
 	// Apply config overrides.
-	if envToken := os.Getenv("SRC_ACCESS_TOKEN"); envToken != "" {
+	if envToken != "" {
 		cfg.AccessToken = envToken
 	}
-	if *endpoint != "" {
-		cfg.Endpoint = *endpoint
-	}
-	if cfg.Endpoint == "" {
-		if endpoint := os.Getenv("SRC_ENDPOINT"); endpoint != "" {
-			cfg.Endpoint = endpoint
-		}
+	if envEndpoint != "" {
+		cfg.Endpoint = envEndpoint
 	}
 	if cfg.Endpoint == "" {
 		cfg.Endpoint = "https://sourcegraph.com"
 	}
 
+	// Lastly, apply endpoint flag if set
+	if endpoint != nil && *endpoint != "" {
+		cfg.Endpoint = *endpoint
+	}
+
 	cfg.Endpoint = strings.TrimSuffix(cfg.Endpoint, "/")
 
 	return &cfg, nil
 }
+
+var errConfigMerge = errors.New("when using a configuration file, zero or all environment variables must be set")

cmd/src/main_test.go

@@ -0,0 +1,179 @@
+package main
+
+import (
+	"encoding/json"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestReadConfig(t *testing.T) {
+	makeTempConfig := func(t *testing.T, c config) (string, func()) {
+		data, err := json.Marshal(c)
+		if err != nil {
+			t.Fatal(err)
+		}
+		tmpDir, err := ioutil.TempDir("", "")
+		if err != nil {
+			t.Fatal(err)
+		}
+		filePath := filepath.Join(tmpDir, "config.json")
+		err = ioutil.WriteFile(filePath, data, 0600)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return filePath, func() { os.RemoveAll(tmpDir) }
+	}
+
+	tests := []struct {
+		name         string
+		fileContents *config
+		envToken     string
+		envEndpoint  string
+		flagEndpoint string
+		want         *config
+		wantErr      string
+	}{
+		{
+			name: "defaults",
+			want: &config{
+				Endpoint: "https://sourcegraph.com",
+			},
+		},
+		{
+			name: "config file, no overrides, trim slash",
+			fileContents: &config{
+				Endpoint:    "https://example.com/",
+				AccessToken: "deadbeef",
+			},
+			want: &config{
+				Endpoint:    "https://example.com",
+				AccessToken: "deadbeef",
+			},
+		},
+		{
+			name: "config file, token override only",
+			fileContents: &config{
+				Endpoint:    "https://example.com/",
+				AccessToken: "deadbeef",
+			},
+			envToken: "abc",
+			want:     nil,
+			wantErr:  errConfigMerge.Error(),
+		},
+		{
+			name: "config file, endpoint override only",
+			fileContents: &config{
+				Endpoint:    "https://example.com/",
+				AccessToken: "deadbeef",
+			},
+			envEndpoint: "https://exmaple2.com",
+			want:        nil,
+			wantErr:     errConfigMerge.Error(),
+		},
+		{
+			name: "config file, both override",
+			fileContents: &config{
+				Endpoint:    "https://example.com/",
+				AccessToken: "deadbeef",
+			},
+			envToken:    "abc",
+			envEndpoint: "https://override.com",
+			want: &config{
+				Endpoint:    "https://override.com",
+				AccessToken: "abc",
+			},
+		},
+		{
+			name:     "no config file, token from environment",
+			envToken: "abc",
+			want: &config{
+				Endpoint:    "https://sourcegraph.com",
+				AccessToken: "abc",
+			},
+		},
+		{
+			name:        "no config file, endpoint from environment",
+			envEndpoint: "https://example.com",
+			want: &config{
+				Endpoint:    "https://example.com",
+				AccessToken: "",
+			},
+		},
+		{
+			name:        "no config file, both variables",
+			envEndpoint: "https://example.com",
+			envToken:    "abc",
+			want: &config{
+				Endpoint:    "https://example.com",
+				AccessToken: "abc",
+			},
+		},
+		{
+			name:         "endpoint flag should override config",
+			flagEndpoint: "https://override.com/",
+			fileContents: &config{
+				Endpoint:    "https://example.com/",
+				AccessToken: "deadbeef",
+			},
+			want: &config{
+				Endpoint:    "https://override.com",
+				AccessToken: "deadbeef",
+			},
+		},
+		{
+			name:         "endpoint flag should override environment",
+			flagEndpoint: "https://override.com/",
+			envEndpoint:  "https://example.com",
+			envToken:     "abc",
+			want: &config{
+				Endpoint:    "https://override.com",
+				AccessToken: "abc",
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			oldConfigPath := *configPath
+			defer func() { *configPath = oldConfigPath }()
+
+			if test.flagEndpoint != "" {
+				val := test.flagEndpoint
+				endpoint = &val
+				defer func() { endpoint = nil }()
+			}
+
+			if test.fileContents != nil {
+				p, cleanup := makeTempConfig(t, *test.fileContents)
+				defer cleanup()
+				*configPath = p
+			}
+			oldToken := os.Getenv("SRC_ACCESS_TOKEN")
+			defer func() { os.Setenv("SRC_ACCESS_TOKEN", oldToken) }()
+			oldEndpoint := os.Getenv("SRC_ENDPOINT")
+			defer func() { os.Setenv("SRC_ENDPOINT", oldEndpoint) }()
+
+			if err := os.Setenv("SRC_ACCESS_TOKEN", test.envToken); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.Setenv("SRC_ENDPOINT", test.envEndpoint); err != nil {
+				t.Fatal(err)
+			}
+			config, err := readConfig()
+			if diff := cmp.Diff(test.want, config); diff != "" {
+				t.Errorf("config: %v", diff)
+			}
+			var errMsg string
+			if err != nil {
+				errMsg = err.Error()
+			}
+			if diff := cmp.Diff(test.wantErr, errMsg); diff != "" {
+				t.Errorf("err: %v", diff)
+			}
+		})
+	}
+}