From 2fbad1d2cc41c136f138517a24e0a4e59e4b22f3 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Wed, 10 Jun 2020 13:03:59 +0200 Subject: [PATCH 1/3] Remove -endpoint flag This change removes the -endpoint flag It also ensures the following: * Both env vars override the config file. * If a config file exists, either zero or both env vars must be specified. It is an error if only one is specified. This is to avoid confusion; we never merge config from the file and env vars. --- cmd/src/main.go | 36 +++++++---- cmd/src/main_test.go | 150 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 13 deletions(-) create mode 100644 cmd/src/main_test.go diff --git a/cmd/src/main.go b/cmd/src/main.go index e3bcc3b642..2480573dc4 100644 --- a/cmd/src/main.go +++ b/cmd/src/main.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "flag" + "github.com/pkg/errors" "io/ioutil" "log" "os" @@ -24,7 +25,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: @@ -48,7 +48,6 @@ Use "src [command] -h" for more information about a command. var ( configPath = flag.String("config", "", "") - endpoint = flag.String("endpoint", "", "") verbose = flag.Bool("v", false, "print verbose output") ) @@ -76,14 +75,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,17 +95,26 @@ 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" @@ -116,3 +124,5 @@ func readConfig() (*config, error) { return &cfg, nil } + +var errConfigMerge = errors.New("config merging not supported, zero or both environment variables must be set") diff --git a/cmd/src/main_test.go b/cmd/src/main_test.go new file mode 100644 index 0000000000..4979631689 --- /dev/null +++ b/cmd/src/main_test.go @@ -0,0 +1,150 @@ +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 + 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", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + oldConfigPath := *configPath + defer func() { *configPath = oldConfigPath }() + + 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) + } + }) + } +} From cbd3f6a23487df9a9ef9f2b8cd07689011c1ab06 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 12 Jun 2020 11:37:48 +0200 Subject: [PATCH 2/3] Add CHANGELOG.md --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000000..45f8fa9cc9 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,22 @@ + + +# Changelog + +All notable changes to `src-cli` are documented in this file. + +## Unreleased + +### Added + +### Changed + +### Fixed + +### Removed + +- Remove the `-endpoint` flag. [#225](https://github.com/sourcegraph/src-cli/pull/225) From cb24c42bf41ad35c319118ad820e3a3e62b08436 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 12 Jun 2020 11:40:06 +0200 Subject: [PATCH 3/3] Add another pending change --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45f8fa9cc9..274ec7cf9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ 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) + ### Changed ### Fixed