From a8c70e43a3936adbce5a15a8421d2245672a8938 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 12 Jun 2017 11:36:49 -0700 Subject: [PATCH 1/5] Move config file loading to more appropriate packages. Signed-off-by: Daniel Nephin --- cli/command/cli.go | 16 +-------------- cli/config/config.go | 24 ++++++++++++++--------- cli/config/config_test.go | 9 --------- cli/config/configfile/file.go | 9 +++++++++ cli/config/configfile/file_test.go | 7 +++++++ cli/config/credentials/file_store_test.go | 3 +-- 6 files changed, 33 insertions(+), 35 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index e56c7a7985..110d242655 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -1,7 +1,6 @@ package command import ( - "fmt" "io" "net/http" "os" @@ -156,7 +155,7 @@ func getConfiguredCredentialStore(c *configfile.ConfigFile, serverAddress string // Initialize the dockerCli runs initialization that must happen after command // line flags are parsed. func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { - cli.configFile = LoadDefaultConfigFile(cli.err) + cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err) var err error cli.client, err = NewAPIClientFromFlags(opts.Common, cli.configFile) @@ -213,19 +212,6 @@ func NewDockerCli(in io.ReadCloser, out, err io.Writer) *DockerCli { return &DockerCli{in: NewInStream(in), out: NewOutStream(out), err: err} } -// LoadDefaultConfigFile attempts to load the default config file and returns -// an initialized ConfigFile struct if none is found. -func LoadDefaultConfigFile(err io.Writer) *configfile.ConfigFile { - configFile, e := cliconfig.Load(cliconfig.Dir()) - if e != nil { - fmt.Fprintf(err, "WARNING: Error loading config file:%v\n", e) - } - if !configFile.ContainsAuth() { - credentials.DetectDefaultStore(configFile) - } - return configFile -} - // NewAPIClientFromFlags creates a new APIClient from command line flags func NewAPIClientFromFlags(opts *cliflags.CommonOptions, configFile *configfile.ConfigFile) (client.APIClient, error) { host, err := getServerHost(opts.Hosts, opts.TLSOptions) diff --git a/cli/config/config.go b/cli/config/config.go index 58c83b6404..74862f1ab2 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -1,11 +1,13 @@ package config import ( + "fmt" "io" "os" "path/filepath" "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/cli/config/credentials" "github.com/docker/docker/api/types" "github.com/docker/docker/pkg/homedir" "github.com/pkg/errors" @@ -38,15 +40,6 @@ func SetDir(dir string) { configDir = dir } -// NewConfigFile initializes an empty configuration file for the given filename 'fn' -func NewConfigFile(fn string) *configfile.ConfigFile { - return &configfile.ConfigFile{ - AuthConfigs: make(map[string]types.AuthConfig), - HTTPHeaders: make(map[string]string), - Filename: fn, - } -} - // LegacyLoadFromReader is a convenience function that creates a ConfigFile object from // a non-nested reader func LegacyLoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) { @@ -118,3 +111,16 @@ func Load(configDir string) (*configfile.ConfigFile, error) { } return &configFile, nil } + +// LoadDefaultConfigFile attempts to load the default config file and returns +// an initialized ConfigFile struct if none is found. +func LoadDefaultConfigFile(err io.Writer) *configfile.ConfigFile { + configFile, e := Load(Dir()) + if e != nil { + fmt.Fprintf(err, "WARNING: Error loading config file:%v\n", e) + } + if !configFile.ContainsAuth() { + credentials.DetectDefaultStore(configFile) + } + return configFile +} diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 7eab2efaa3..4f98df9bff 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -484,15 +484,6 @@ func TestConfigDir(t *testing.T) { } } -func TestConfigFile(t *testing.T) { - configFilename := "configFilename" - configFile := NewConfigFile(configFilename) - - if configFile.Filename != configFilename { - t.Fatalf("Expected %s, got %s", configFilename, configFile.Filename) - } -} - func TestJSONReaderNoFile(t *testing.T) { js := ` { "auths": { "https://index.docker.io/v1/": { "auth": "am9lam9lOmhlbGxv", "email": "user@example.com" } } }` diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index 9c2c4eec6d..e6dd13934c 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -53,6 +53,15 @@ type ProxyConfig struct { FTPProxy string `json:"ftpProxy,omitempty"` } +// NewConfigFile initializes an empty configuration file for the given filename 'fn' +func NewConfigFile(fn string) *ConfigFile { + return &ConfigFile{ + AuthConfigs: make(map[string]types.AuthConfig), + HTTPHeaders: make(map[string]string), + Filename: fn, + } +} + // LegacyLoadFromReader reads the non-nested configuration data given and sets up the // auth config information with given directory and populates the receiver object func (configFile *ConfigFile) LegacyLoadFromReader(configData io.Reader) error { diff --git a/cli/config/configfile/file_test.go b/cli/config/configfile/file_test.go index 8c84347719..ac3271006e 100644 --- a/cli/config/configfile/file_test.go +++ b/cli/config/configfile/file_test.go @@ -142,3 +142,10 @@ func TestProxyConfigPerHost(t *testing.T) { } assert.Equal(t, expected, proxyConfig) } + +func TestConfigFile(t *testing.T) { + configFilename := "configFilename" + configFile := NewConfigFile(configFilename) + + assert.Equal(t, configFilename, configFile.Filename) +} diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index 6e16f2cd32..eadbcc23ef 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -4,7 +4,6 @@ import ( "io/ioutil" "testing" - cliconfig "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker/api/types" ) @@ -14,7 +13,7 @@ func newConfigFile(auths map[string]types.AuthConfig) *configfile.ConfigFile { name := tmp.Name() tmp.Close() - c := cliconfig.NewConfigFile(name) + c := configfile.NewConfigFile(name) c.AuthConfigs = auths return c } From 33cbb70270b6d930579fa47c1a069f643dab52a5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 21 Jun 2017 16:47:06 -0400 Subject: [PATCH 2/5] Cleanup config/credentials, remove dependency on config file. Signed-off-by: Daniel Nephin --- cli/config/config.go | 2 +- cli/config/configfile/file.go | 5 + cli/config/credentials/default_store.go | 21 ++- cli/config/credentials/file_store.go | 24 +-- cli/config/credentials/file_store_test.go | 60 ++++--- cli/config/credentials/native_store.go | 3 +- cli/config/credentials/native_store_test.go | 168 ++++++-------------- 7 files changed, 101 insertions(+), 182 deletions(-) diff --git a/cli/config/config.go b/cli/config/config.go index 74862f1ab2..ce296a6b13 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -120,7 +120,7 @@ func LoadDefaultConfigFile(err io.Writer) *configfile.ConfigFile { fmt.Fprintf(err, "WARNING: Error loading config file:%v\n", e) } if !configFile.ContainsAuth() { - credentials.DetectDefaultStore(configFile) + configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore) } return configFile } diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index e6dd13934c..0be6e70b63 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -127,6 +127,11 @@ func (configFile *ConfigFile) ContainsAuth() bool { len(configFile.AuthConfigs) > 0 } +// GetAuthConfigs returns the mapping of repo to auth configuration +func (configFile *ConfigFile) GetAuthConfigs() map[string]types.AuthConfig { + return configFile.AuthConfigs +} + // SaveToWriter encodes and writes out all the authorization information to // the given writer func (configFile *ConfigFile) SaveToWriter(writer io.Writer) error { diff --git a/cli/config/credentials/default_store.go b/cli/config/credentials/default_store.go index dc080dbd40..b2cc4df8bc 100644 --- a/cli/config/credentials/default_store.go +++ b/cli/config/credentials/default_store.go @@ -2,21 +2,18 @@ package credentials import ( "os/exec" - - "github.com/docker/cli/cli/config/configfile" ) -// DetectDefaultStore sets the default credentials store -// if the host includes the default store helper program. -func DetectDefaultStore(c *configfile.ConfigFile) { - if c.CredentialsStore != "" { - // user defined - return +// DetectDefaultStore return the default credentials store for the platform if +// the store executable is available. +func DetectDefaultStore(store string) string { + // user defined or no default for platform + if store != "" || defaultCredentialsStore == "" { + return store } - if defaultCredentialsStore != "" { - if _, err := exec.LookPath(remoteCredentialsPrefix + defaultCredentialsStore); err == nil { - c.CredentialsStore = defaultCredentialsStore - } + if _, err := exec.LookPath(remoteCredentialsPrefix + defaultCredentialsStore); err == nil { + return defaultCredentialsStore } + return "" } diff --git a/cli/config/credentials/file_store.go b/cli/config/credentials/file_store.go index 4e3325c79b..b186dbbed6 100644 --- a/cli/config/credentials/file_store.go +++ b/cli/config/credentials/file_store.go @@ -1,37 +1,39 @@ package credentials import ( - "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker/api/types" "github.com/docker/docker/registry" ) +type store interface { + Save() error + GetAuthConfigs() map[string]types.AuthConfig +} + // fileStore implements a credentials store using // the docker configuration file to keep the credentials in plain text. type fileStore struct { - file *configfile.ConfigFile + file store } // NewFileStore creates a new file credentials store. -func NewFileStore(file *configfile.ConfigFile) Store { - return &fileStore{ - file: file, - } +func NewFileStore(file store) Store { + return &fileStore{file: file} } // Erase removes the given credentials from the file store. func (c *fileStore) Erase(serverAddress string) error { - delete(c.file.AuthConfigs, serverAddress) + delete(c.file.GetAuthConfigs(), serverAddress) return c.file.Save() } // Get retrieves credentials for a specific server from the file store. func (c *fileStore) Get(serverAddress string) (types.AuthConfig, error) { - authConfig, ok := c.file.AuthConfigs[serverAddress] + authConfig, ok := c.file.GetAuthConfigs()[serverAddress] if !ok { // Maybe they have a legacy config file, we will iterate the keys converting // them to the new format and testing - for r, ac := range c.file.AuthConfigs { + for r, ac := range c.file.GetAuthConfigs() { if serverAddress == registry.ConvertToHostname(r) { return ac, nil } @@ -43,11 +45,11 @@ func (c *fileStore) Get(serverAddress string) (types.AuthConfig, error) { } func (c *fileStore) GetAll() (map[string]types.AuthConfig, error) { - return c.file.AuthConfigs, nil + return c.file.GetAuthConfigs(), nil } // Store saves the given credentials in the file store. func (c *fileStore) Store(authConfig types.AuthConfig) error { - c.file.AuthConfigs[authConfig.ServerAddress] = authConfig + c.file.GetAuthConfigs()[authConfig.ServerAddress] = authConfig return c.file.Save() } diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index eadbcc23ef..0d4fbd6bdc 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -1,55 +1,49 @@ package credentials import ( - "io/ioutil" "testing" - "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker/api/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func newConfigFile(auths map[string]types.AuthConfig) *configfile.ConfigFile { - tmp, _ := ioutil.TempFile("", "docker-test") - name := tmp.Name() - tmp.Close() +type fakeStore struct { + configs map[string]types.AuthConfig +} - c := configfile.NewConfigFile(name) - c.AuthConfigs = auths - return c +func (f *fakeStore) Save() error { + return nil +} + +func (f *fakeStore) GetAuthConfigs() map[string]types.AuthConfig { + return f.configs +} + +func newStore(auths map[string]types.AuthConfig) store { + return &fakeStore{configs: auths} } func TestFileStoreAddCredentials(t *testing.T) { - f := newConfigFile(make(map[string]types.AuthConfig)) + f := newStore(make(map[string]types.AuthConfig)) s := NewFileStore(f) - err := s.Store(types.AuthConfig{ + auth := types.AuthConfig{ Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", - }) + } + err := s.Store(auth) + require.NoError(t, err) + assert.Len(t, f.GetAuthConfigs(), 1) - if err != nil { - t.Fatal(err) - } - - if len(f.AuthConfigs) != 1 { - t.Fatalf("expected 1 auth config, got %d", len(f.AuthConfigs)) - } - - a, ok := f.AuthConfigs["https://example.com"] - if !ok { - t.Fatalf("expected auth for https://example.com, got %v", f.AuthConfigs) - } - if a.Auth != "super_secret_token" { - t.Fatalf("expected auth `super_secret_token`, got %s", a.Auth) - } - if a.Email != "foo@example.com" { - t.Fatalf("expected email `foo@example.com`, got %s", a.Email) - } + actual, ok := f.GetAuthConfigs()["https://example.com"] + assert.True(t, ok) + assert.Equal(t, auth, actual) } func TestFileStoreGet(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", @@ -73,7 +67,7 @@ func TestFileStoreGet(t *testing.T) { func TestFileStoreGetAll(t *testing.T) { s1 := "https://example.com" s2 := "https://example2.com" - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ s1: { Auth: "super_secret_token", Email: "foo@example.com", @@ -109,7 +103,7 @@ func TestFileStoreGetAll(t *testing.T) { } func TestFileStoreErase(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", diff --git a/cli/config/credentials/native_store.go b/cli/config/credentials/native_store.go index cef34db92f..ef3aab4ad3 100644 --- a/cli/config/credentials/native_store.go +++ b/cli/config/credentials/native_store.go @@ -1,7 +1,6 @@ package credentials import ( - "github.com/docker/cli/cli/config/configfile" "github.com/docker/docker-credential-helpers/client" "github.com/docker/docker-credential-helpers/credentials" "github.com/docker/docker/api/types" @@ -22,7 +21,7 @@ type nativeStore struct { // NewNativeStore creates a new native store that // uses a remote helper program to manage credentials. -func NewNativeStore(file *configfile.ConfigFile, helperSuffix string) Store { +func NewNativeStore(file store, helperSuffix string) Store { name := remoteCredentialsPrefix + helperSuffix return &nativeStore{ programFunc: client.NewShellProgramFunc(name), diff --git a/cli/config/credentials/native_store_test.go b/cli/config/credentials/native_store_test.go index 360cc20efc..582c2bd598 100644 --- a/cli/config/credentials/native_store_test.go +++ b/cli/config/credentials/native_store_test.go @@ -11,7 +11,10 @@ import ( "github.com/docker/docker-credential-helpers/client" "github.com/docker/docker-credential-helpers/credentials" "github.com/docker/docker/api/types" + "github.com/docker/docker/pkg/testutil" "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -90,53 +93,32 @@ func mockCommandFn(args ...string) client.Program { } func TestNativeStoreAddCredentials(t *testing.T) { - f := newConfigFile(make(map[string]types.AuthConfig)) - f.CredentialsStore = "mock" - + f := newStore(make(map[string]types.AuthConfig)) s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } - err := s.Store(types.AuthConfig{ + auth := types.AuthConfig{ Username: "foo", Password: "bar", Email: "foo@example.com", ServerAddress: validServerAddress, - }) + } + err := s.Store(auth) + require.NoError(t, err) + assert.Len(t, f.GetAuthConfigs(), 1) - if err != nil { - t.Fatal(err) - } - - if len(f.AuthConfigs) != 1 { - t.Fatalf("expected 1 auth config, got %d", len(f.AuthConfigs)) - } - - a, ok := f.AuthConfigs[validServerAddress] - if !ok { - t.Fatalf("expected auth for %s, got %v", validServerAddress, f.AuthConfigs) - } - if a.Auth != "" { - t.Fatalf("expected auth to be empty, got %s", a.Auth) - } - if a.Username != "" { - t.Fatalf("expected username to be empty, got %s", a.Username) - } - if a.Password != "" { - t.Fatalf("expected password to be empty, got %s", a.Password) - } - if a.IdentityToken != "" { - t.Fatalf("expected identity token to be empty, got %s", a.IdentityToken) - } - if a.Email != "foo@example.com" { - t.Fatalf("expected email `foo@example.com`, got %s", a.Email) + actual, ok := f.GetAuthConfigs()[validServerAddress] + assert.True(t, ok) + expected := types.AuthConfig{ + Email: auth.Email, + ServerAddress: auth.ServerAddress, } + assert.Equal(t, expected, actual) } func TestNativeStoreAddInvalidCredentials(t *testing.T) { - f := newConfigFile(make(map[string]types.AuthConfig)) - f.CredentialsStore = "mock" - + f := newStore(make(map[string]types.AuthConfig)) s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -147,102 +129,66 @@ func TestNativeStoreAddInvalidCredentials(t *testing.T) { Email: "foo@example.com", ServerAddress: invalidServerAddress, }) - - if err == nil { - t.Fatal("expected error, got nil") - } - - if !strings.Contains(err.Error(), "program failed") { - t.Fatalf("expected `program failed`, got %v", err) - } - - if len(f.AuthConfigs) != 0 { - t.Fatalf("expected 0 auth config, got %d", len(f.AuthConfigs)) - } + testutil.ErrorContains(t, err, "program failed") + assert.Len(t, f.GetAuthConfigs(), 0) } func TestNativeStoreGet(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" - s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } - a, err := s.Get(validServerAddress) - if err != nil { - t.Fatal(err) - } + actual, err := s.Get(validServerAddress) + require.NoError(t, err) - if a.Username != "foo" { - t.Fatalf("expected username `foo`, got %s", a.Username) - } - if a.Password != "bar" { - t.Fatalf("expected password `bar`, got %s", a.Password) - } - if a.IdentityToken != "" { - t.Fatalf("expected identity token to be empty, got %s", a.IdentityToken) - } - if a.Email != "foo@example.com" { - t.Fatalf("expected email `foo@example.com`, got %s", a.Email) + expected := types.AuthConfig{ + Username: "foo", + Password: "bar", + Email: "foo@example.com", } + assert.Equal(t, expected, actual) } func TestNativeStoreGetIdentityToken(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress2: { Email: "foo@example2.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } - a, err := s.Get(validServerAddress2) - if err != nil { - t.Fatal(err) - } + actual, err := s.Get(validServerAddress2) + require.NoError(t, err) - if a.Username != "" { - t.Fatalf("expected username to be empty, got %s", a.Username) - } - if a.Password != "" { - t.Fatalf("expected password to be empty, got %s", a.Password) - } - if a.IdentityToken != "abcd1234" { - t.Fatalf("expected identity token `abcd1234`, got %s", a.IdentityToken) - } - if a.Email != "foo@example2.com" { - t.Fatalf("expected email `foo@example2.com`, got %s", a.Email) + expected := types.AuthConfig{ + IdentityToken: "abcd1234", + Email: "foo@example2.com", } + assert.Equal(t, expected, actual) } func TestNativeStoreGetAll(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } as, err := s.GetAll() - if err != nil { - t.Fatal(err) - } - - if len(as) != 2 { - t.Fatalf("wanted 2, got %d", len(as)) - } + require.NoError(t, err) + assert.Len(t, as, 2) if as[validServerAddress].Username != "foo" { t.Fatalf("expected username `foo` for %s, got %s", validServerAddress, as[validServerAddress].Username) @@ -271,86 +217,62 @@ func TestNativeStoreGetAll(t *testing.T) { } func TestNativeStoreGetMissingCredentials(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } _, err := s.Get(missingCredsAddress) - if err != nil { - // missing credentials do not produce an error - t.Fatal(err) - } + assert.NoError(t, err) } func TestNativeStoreGetInvalidAddress(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } _, err := s.Get(invalidServerAddress) - if err == nil { - t.Fatal("expected error, got nil") - } - - if !strings.Contains(err.Error(), "program failed") { - t.Fatalf("expected `program failed`, got %v", err) - } + testutil.ErrorContains(t, err, "program failed") } func TestNativeStoreErase(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } err := s.Erase(validServerAddress) - if err != nil { - t.Fatal(err) - } - - if len(f.AuthConfigs) != 0 { - t.Fatalf("expected 0 auth configs, got %d", len(f.AuthConfigs)) - } + require.NoError(t, err) + assert.Len(t, f.GetAuthConfigs(), 0) } func TestNativeStoreEraseInvalidAddress(t *testing.T) { - f := newConfigFile(map[string]types.AuthConfig{ + f := newStore(map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, }) - f.CredentialsStore = "mock" s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), } err := s.Erase(invalidServerAddress) - if err == nil { - t.Fatal("expected error, got nil") - } - - if !strings.Contains(err.Error(), "program failed") { - t.Fatalf("expected `program failed`, got %v", err) - } + testutil.ErrorContains(t, err, "program failed") } From a3cbc701474dc7b48d80f78a1f93604c85b7672d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 21 Jun 2017 17:20:49 -0400 Subject: [PATCH 3/5] Move credential getting functions to the ConfigFile. Signed-off-by: Daniel Nephin --- cli/command/cli.go | 52 -------------------------------- cli/command/image/build.go | 9 +++--- cli/command/image/cmd.go | 3 +- cli/command/image/pull_test.go | 9 ++++-- cli/command/image/push_test.go | 11 +++++-- cli/command/registry.go | 4 +-- cli/command/registry/login.go | 2 +- cli/command/registry/logout.go | 2 +- cli/config/config.go | 8 ++--- cli/config/configfile/file.go | 54 ++++++++++++++++++++++++++++++++++ cli/internal/test/cli.go | 8 ----- 11 files changed, 83 insertions(+), 79 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index 110d242655..885859abc9 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -9,11 +9,9 @@ import ( "github.com/docker/cli/cli" cliconfig "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" - "github.com/docker/cli/cli/config/credentials" cliflags "github.com/docker/cli/cli/flags" dopts "github.com/docker/cli/opts" "github.com/docker/docker/api" - "github.com/docker/docker/api/types" "github.com/docker/docker/client" "github.com/docker/go-connections/sockets" "github.com/docker/go-connections/tlsconfig" @@ -38,7 +36,6 @@ type Cli interface { In() *InStream SetIn(in *InStream) ConfigFile() *configfile.ConfigFile - CredentialsStore(serverAddress string) credentials.Store } // DockerCli is an instance the docker command line client. @@ -103,55 +100,6 @@ func (cli *DockerCli) ServerInfo() ServerInfo { return cli.server } -// GetAllCredentials returns all of the credentials stored in all of the -// configured credential stores. -func (cli *DockerCli) GetAllCredentials() (map[string]types.AuthConfig, error) { - auths := make(map[string]types.AuthConfig) - for registry := range cli.configFile.CredentialHelpers { - helper := cli.CredentialsStore(registry) - newAuths, err := helper.GetAll() - if err != nil { - return nil, err - } - addAll(auths, newAuths) - } - defaultStore := cli.CredentialsStore("") - newAuths, err := defaultStore.GetAll() - if err != nil { - return nil, err - } - addAll(auths, newAuths) - return auths, nil -} - -func addAll(to, from map[string]types.AuthConfig) { - for reg, ac := range from { - to[reg] = ac - } -} - -// CredentialsStore returns a new credentials store based -// on the settings provided in the configuration file. Empty string returns -// the default credential store. -func (cli *DockerCli) CredentialsStore(serverAddress string) credentials.Store { - if helper := getConfiguredCredentialStore(cli.configFile, serverAddress); helper != "" { - return credentials.NewNativeStore(cli.configFile, helper) - } - return credentials.NewFileStore(cli.configFile) -} - -// getConfiguredCredentialStore returns the credential helper configured for the -// given registry, the default credsStore, or the empty string if neither are -// configured. -func getConfiguredCredentialStore(c *configfile.ConfigFile, serverAddress string) string { - if c.CredentialHelpers != nil && serverAddress != "" { - if helper, exists := c.CredentialHelpers[serverAddress]; exists { - return helper - } - } - return c.CredentialsStore -} - // Initialize the dockerCli runs initialization that must happen after command // line flags are parsed. func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { diff --git a/cli/command/image/build.go b/cli/command/image/build.go index a05970b8b0..3d595e0393 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -78,7 +78,7 @@ func (o buildOptions) contextFromStdin() bool { } // NewBuildCommand creates a new `docker build` command -func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command { +func NewBuildCommand(dockerCli command.Cli) *cobra.Command { ulimits := make(map[string]*units.Ulimit) options := buildOptions{ tags: opts.NewListOpts(validateTag), @@ -159,7 +159,7 @@ func (out *lastProgressOutput) WriteProgress(prog progress.Progress) error { } // nolint: gocyclo -func runBuild(dockerCli *command.DockerCli, options buildOptions) error { +func runBuild(dockerCli command.Cli, options buildOptions) error { var ( buildCtx io.ReadCloser dockerfileCtx io.ReadCloser @@ -336,7 +336,8 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error { body = buildCtx } - authConfigs, _ := dockerCli.GetAllCredentials() + configFile := dockerCli.ConfigFile() + authConfigs, _ := configFile.GetAllCredentials() buildOptions := types.ImageBuildOptions{ Memory: options.memory.Value(), MemorySwap: options.memorySwap.Value(), @@ -356,7 +357,7 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error { Dockerfile: relDockerfile, ShmSize: options.shmSize.Value(), Ulimits: options.ulimits.GetList(), - BuildArgs: dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), options.buildArgs.GetAll()), + BuildArgs: configFile.ParseProxyConfig(dockerCli.Client().DaemonHost(), options.buildArgs.GetAll()), AuthConfigs: authConfigs, Labels: opts.ConvertKVStringsToMap(options.labels.GetAll()), CacheFrom: options.cacheFrom, diff --git a/cli/command/image/cmd.go b/cli/command/image/cmd.go index 10357fcfd8..a12bf3395b 100644 --- a/cli/command/image/cmd.go +++ b/cli/command/image/cmd.go @@ -8,8 +8,7 @@ import ( ) // NewImageCommand returns a cobra command for `image` subcommands -// nolint: interfacer -func NewImageCommand(dockerCli *command.DockerCli) *cobra.Command { +func NewImageCommand(dockerCli command.Cli) *cobra.Command { cmd := &cobra.Command{ Use: "image", Short: "Manage images", diff --git a/cli/command/image/pull_test.go b/cli/command/image/pull_test.go index d72531b768..731c774a1c 100644 --- a/cli/command/image/pull_test.go +++ b/cli/command/image/pull_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "testing" + "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/internal/test" "github.com/docker/docker/pkg/testutil" "github.com/docker/docker/pkg/testutil/golden" @@ -41,7 +42,9 @@ func TestNewPullCommandErrors(t *testing.T) { } for _, tc := range testCases { buf := new(bytes.Buffer) - cmd := NewPullCommand(test.NewFakeCli(&fakeClient{}, buf)) + cli := test.NewFakeCli(&fakeClient{}, buf) + cli.SetConfigfile(configfile.NewConfigFile("filename")) + cmd := NewPullCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) testutil.ErrorContains(t, cmd.Execute(), tc.expectedError) @@ -64,7 +67,9 @@ func TestNewPullCommandSuccess(t *testing.T) { } for _, tc := range testCases { buf := new(bytes.Buffer) - cmd := NewPullCommand(test.NewFakeCli(&fakeClient{}, buf)) + cli := test.NewFakeCli(&fakeClient{}, buf) + cli.SetConfigfile(configfile.NewConfigFile("filename")) + cmd := NewPullCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) err := cmd.Execute() diff --git a/cli/command/image/push_test.go b/cli/command/image/push_test.go index 559b1b89c3..f02e0407d6 100644 --- a/cli/command/image/push_test.go +++ b/cli/command/image/push_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/internal/test" "github.com/docker/docker/api/types" "github.com/docker/docker/pkg/testutil" @@ -47,7 +48,9 @@ func TestNewPushCommandErrors(t *testing.T) { } for _, tc := range testCases { buf := new(bytes.Buffer) - cmd := NewPushCommand(test.NewFakeCli(&fakeClient{imagePushFunc: tc.imagePushFunc}, buf)) + cli := test.NewFakeCli(&fakeClient{imagePushFunc: tc.imagePushFunc}, buf) + cli.SetConfigfile(configfile.NewConfigFile("filename")) + cmd := NewPushCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) testutil.ErrorContains(t, cmd.Execute(), tc.expectedError) @@ -66,11 +69,13 @@ func TestNewPushCommandSuccess(t *testing.T) { } for _, tc := range testCases { buf := new(bytes.Buffer) - cmd := NewPushCommand(test.NewFakeCli(&fakeClient{ + cli := test.NewFakeCli(&fakeClient{ imagePushFunc: func(ref string, options types.ImagePushOptions) (io.ReadCloser, error) { return ioutil.NopCloser(strings.NewReader("")), nil }, - }, buf)) + }, buf) + cli.SetConfigfile(configfile.NewConfigFile("filename")) + cmd := NewPushCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) assert.NoError(t, cmd.Execute()) diff --git a/cli/command/registry.go b/cli/command/registry.go index 802b3a4b83..f3958d8a46 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -70,7 +70,7 @@ func ResolveAuthConfig(ctx context.Context, cli Cli, index *registrytypes.IndexI configKey = ElectAuthServer(ctx, cli) } - a, _ := cli.CredentialsStore(configKey).Get(configKey) + a, _ := cli.ConfigFile().GetAuthConfig(configKey) return a } @@ -85,7 +85,7 @@ func ConfigureAuth(cli Cli, flUser, flPassword, serverAddress string, isDefaultR serverAddress = registry.ConvertToHostname(serverAddress) } - authconfig, err := cli.CredentialsStore(serverAddress).Get(serverAddress) + authconfig, err := cli.ConfigFile().GetAuthConfig(serverAddress) if err != nil { return authconfig, err } diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index ba1b133054..4ffca2bed2 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -71,7 +71,7 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error { authConfig.Password = "" authConfig.IdentityToken = response.IdentityToken } - if err := dockerCli.CredentialsStore(serverAddress).Store(authConfig); err != nil { + if err := dockerCli.ConfigFile().GetCredentialsStore(serverAddress).Store(authConfig); err != nil { return errors.Errorf("Error saving credentials: %v", err) } diff --git a/cli/command/registry/logout.go b/cli/command/registry/logout.go index d241df4cf0..cce626e58a 100644 --- a/cli/command/registry/logout.go +++ b/cli/command/registry/logout.go @@ -68,7 +68,7 @@ func runLogout(dockerCli command.Cli, serverAddress string) error { fmt.Fprintf(dockerCli.Out(), "Removing login credentials for %s\n", hostnameAddress) for _, r := range regsToLogout { - if err := dockerCli.CredentialsStore(r).Erase(r); err != nil { + if err := dockerCli.ConfigFile().GetCredentialsStore(r).Erase(r); err != nil { fmt.Fprintf(dockerCli.Err(), "WARNING: could not erase credentials: %v\n", err) } } diff --git a/cli/config/config.go b/cli/config/config.go index ce296a6b13..b054dc49fa 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -114,10 +114,10 @@ func Load(configDir string) (*configfile.ConfigFile, error) { // LoadDefaultConfigFile attempts to load the default config file and returns // an initialized ConfigFile struct if none is found. -func LoadDefaultConfigFile(err io.Writer) *configfile.ConfigFile { - configFile, e := Load(Dir()) - if e != nil { - fmt.Fprintf(err, "WARNING: Error loading config file:%v\n", e) +func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile { + configFile, err := Load(Dir()) + if err != nil { + fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err) } if !configFile.ContainsAuth() { configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore) diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index 0be6e70b63..d599cc5aa9 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "github.com/docker/cli/cli/config/credentials" "github.com/docker/cli/opts" "github.com/docker/docker/api/types" "github.com/pkg/errors" @@ -245,3 +246,56 @@ func decodeAuth(authStr string) (string, string, error) { password := strings.Trim(arr[1], "\x00") return arr[0], password, nil } + +// GetCredentialsStore returns a new credentials store from the settings in the +// configuration file +func (configFile *ConfigFile) GetCredentialsStore(serverAddress string) credentials.Store { + if helper := getConfiguredCredentialStore(configFile, serverAddress); helper != "" { + return credentials.NewNativeStore(configFile, helper) + } + return credentials.NewFileStore(configFile) +} + +// GetAuthConfig for a repository from the credential store +func (configFile *ConfigFile) GetAuthConfig(serverAddress string) (types.AuthConfig, error) { + return configFile.GetCredentialsStore(serverAddress).Get(serverAddress) +} + +// getConfiguredCredentialStore returns the credential helper configured for the +// given registry, the default credsStore, or the empty string if neither are +// configured. +func getConfiguredCredentialStore(c *ConfigFile, serverAddress string) string { + if c.CredentialHelpers != nil && serverAddress != "" { + if helper, exists := c.CredentialHelpers[serverAddress]; exists { + return helper + } + } + return c.CredentialsStore +} + +// GetAllCredentials returns all of the credentials stored in all of the +// configured credential stores. +func (configFile *ConfigFile) GetAllCredentials() (map[string]types.AuthConfig, error) { + auths := make(map[string]types.AuthConfig) + addAll := func(from map[string]types.AuthConfig) { + for reg, ac := range from { + auths[reg] = ac + } + } + + for registry := range configFile.CredentialHelpers { + helper := configFile.GetCredentialsStore(registry) + newAuths, err := helper.GetAll() + if err != nil { + return nil, err + } + addAll(newAuths) + } + defaultStore := configFile.GetCredentialsStore("") + newAuths, err := defaultStore.GetAll() + if err != nil { + return nil, err + } + addAll(newAuths) + return auths, nil +} diff --git a/cli/internal/test/cli.go b/cli/internal/test/cli.go index f0f75f7bd5..fa77a86a31 100644 --- a/cli/internal/test/cli.go +++ b/cli/internal/test/cli.go @@ -71,11 +71,3 @@ func (c *FakeCli) In() *command.InStream { func (c *FakeCli) ConfigFile() *configfile.ConfigFile { return c.configfile } - -// CredentialsStore returns the fake store the cli will use -func (c *FakeCli) CredentialsStore(serverAddress string) credentials.Store { - if c.store == nil { - c.store = NewFakeStore() - } - return c.store -} From 62dfbef4d81638f2d3e579520cd533c0b7789919 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 22 Jun 2017 12:03:58 -0400 Subject: [PATCH 4/5] Add missing unit tests. Signed-off-by: Daniel Nephin --- cli/config/config.go | 30 ++-- cli/config/config_test.go | 249 +++++++++++------------------ cli/config/configfile/file_test.go | 36 +++-- 3 files changed, 125 insertions(+), 190 deletions(-) diff --git a/cli/config/config.go b/cli/config/config.go index b054dc49fa..5e638a519f 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -68,48 +68,42 @@ func Load(configDir string) (*configfile.ConfigFile, error) { configDir = Dir() } - configFile := configfile.ConfigFile{ - AuthConfigs: make(map[string]types.AuthConfig), - Filename: filepath.Join(configDir, ConfigFileName), - } + filename := filepath.Join(configDir, ConfigFileName) + configFile := configfile.NewConfigFile(filename) // Try happy path first - latest config file - if _, err := os.Stat(configFile.Filename); err == nil { - file, err := os.Open(configFile.Filename) + if _, err := os.Stat(filename); err == nil { + file, err := os.Open(filename) if err != nil { - return &configFile, errors.Errorf("%s - %v", configFile.Filename, err) + return configFile, errors.Errorf("%s - %v", filename, err) } defer file.Close() err = configFile.LoadFromReader(file) if err != nil { - err = errors.Errorf("%s - %v", configFile.Filename, err) + err = errors.Errorf("%s - %v", filename, err) } - return &configFile, err + return configFile, err } else if !os.IsNotExist(err) { // if file is there but we can't stat it for any reason other // than it doesn't exist then stop - return &configFile, errors.Errorf("%s - %v", configFile.Filename, err) + return configFile, errors.Errorf("%s - %v", filename, err) } // Can't find latest config file so check for the old one confFile := filepath.Join(homedir.Get(), oldConfigfile) if _, err := os.Stat(confFile); err != nil { - return &configFile, nil //missing file is not an error + return configFile, nil //missing file is not an error } file, err := os.Open(confFile) if err != nil { - return &configFile, errors.Errorf("%s - %v", confFile, err) + return configFile, errors.Errorf("%s - %v", confFile, err) } defer file.Close() err = configFile.LegacyLoadFromReader(file) if err != nil { - return &configFile, errors.Errorf("%s - %v", confFile, err) + return configFile, errors.Errorf("%s - %v", confFile, err) } - - if configFile.HTTPHeaders == nil { - configFile.HTTPHeaders = map[string]string{} - } - return &configFile, nil + return configFile, nil } // LoadDefaultConfigFile attempts to load the default config file and returns diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 4f98df9bff..1248913c5e 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "io/ioutil" "os" "path/filepath" @@ -8,28 +9,34 @@ import ( "testing" "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/cli/config/credentials" "github.com/docker/docker/pkg/homedir" + "github.com/docker/docker/pkg/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestEmptyConfigDir(t *testing.T) { - tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpHome) +func setupConfigDir(t *testing.T) (string, func()) { + tmpdir, err := ioutil.TempDir("", "config-test") + require.NoError(t, err) + oldDir := Dir() + SetDir(tmpdir) - SetDir(tmpHome) + return tmpdir, func() { + SetDir(oldDir) + os.RemoveAll(tmpdir) + } +} + +func TestEmptyConfigDir(t *testing.T) { + tmpHome, cleanup := setupConfigDir(t) + defer cleanup() config, err := Load("") - if err != nil { - t.Fatalf("Failed loading on empty config dir: %q", err) - } + require.NoError(t, err) expectedConfigFilename := filepath.Join(tmpHome, ConfigFileName) - if config.Filename != expectedConfigFilename { - t.Fatalf("Expected config filename %s, got %s", expectedConfigFilename, config.Filename) - } + assert.Equal(t, expectedConfigFilename, config.Filename) // Now save it and make sure it shows up in new form saveConfigAndValidateNewFormat(t, config, tmpHome) @@ -37,15 +44,11 @@ func TestEmptyConfigDir(t *testing.T) { func TestMissingFile(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on missing file: %q", err) - } + require.NoError(t, err) // Now save it and make sure it shows up in new form saveConfigAndValidateNewFormat(t, config, tmpHome) @@ -53,17 +56,13 @@ func TestMissingFile(t *testing.T) { func TestSaveFileToDirs(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) tmpHome += "/.docker" config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on missing file: %q", err) - } + require.NoError(t, err) // Now save it and make sure it shows up in new form saveConfigAndValidateNewFormat(t, config, tmpHome) @@ -71,38 +70,28 @@ func TestSaveFileToDirs(t *testing.T) { func TestEmptyFile(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) - if err := ioutil.WriteFile(fn, []byte(""), 0600); err != nil { - t.Fatal(err) - } + err = ioutil.WriteFile(fn, []byte(""), 0600) + require.NoError(t, err) _, err = Load(tmpHome) - if err == nil { - t.Fatalf("Was supposed to fail") - } + testutil.ErrorContains(t, err, "EOF") } func TestEmptyJSON(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) - if err := ioutil.WriteFile(fn, []byte("{}"), 0600); err != nil { - t.Fatal(err) - } + err = ioutil.WriteFile(fn, []byte("{}"), 0600) + require.NoError(t, err) config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) // Now save it and make sure it shows up in new form saveConfigAndValidateNewFormat(t, config, tmpHome) @@ -118,9 +107,7 @@ email`: "Invalid auth configuration file", } tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) homeKey := homedir.Key() @@ -131,27 +118,17 @@ email`: "Invalid auth configuration file", for content, expectedError := range invalids { fn := filepath.Join(tmpHome, oldConfigfile) - if err := ioutil.WriteFile(fn, []byte(content), 0600); err != nil { - t.Fatal(err) - } - - config, err := Load(tmpHome) - // Use Contains instead of == since the file name will change each time - if err == nil || !strings.Contains(err.Error(), expectedError) { - t.Fatalf("Should have failed\nConfig: %v\nGot: %v\nExpected: %v", config, err, expectedError) - } + err := ioutil.WriteFile(fn, []byte(content), 0600) + require.NoError(t, err) + _, err = Load(tmpHome) + testutil.ErrorContains(t, err, expectedError) } } func TestOldValidAuth(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) homeKey := homedir.Key() @@ -163,14 +140,11 @@ func TestOldValidAuth(t *testing.T) { fn := filepath.Join(tmpHome, oldConfigfile) js := `username = am9lam9lOmhlbGxv email = user@example.com` - if err := ioutil.WriteFile(fn, []byte(js), 0600); err != nil { - t.Fatal(err) - } + err = ioutil.WriteFile(fn, []byte(js), 0600) + require.NoError(t, err) config, err := Load(tmpHome) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) // defaultIndexserver is https://index.docker.io/v1/ ac := config.AuthConfigs["https://index.docker.io/v1/"] @@ -189,16 +163,12 @@ func TestOldValidAuth(t *testing.T) { } }` - if configStr != expConfStr { - t.Fatalf("Should have save in new form: \n%s\n not \n%s", configStr, expConfStr) - } + assert.Equal(t, expConfStr, configStr) } func TestOldJSONInvalid(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) homeKey := homedir.Key() @@ -222,9 +192,7 @@ func TestOldJSONInvalid(t *testing.T) { func TestOldJSON(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) homeKey := homedir.Key() @@ -240,9 +208,7 @@ func TestOldJSON(t *testing.T) { } config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) ac := config.AuthConfigs["https://index.docker.io/v1/"] if ac.Username != "joejoe" || ac.Password != "hello" { @@ -268,9 +234,7 @@ func TestOldJSON(t *testing.T) { func TestNewJSON(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) @@ -280,9 +244,7 @@ func TestNewJSON(t *testing.T) { } config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) ac := config.AuthConfigs["https://index.docker.io/v1/"] if ac.Username != "joejoe" || ac.Password != "hello" { @@ -307,9 +269,7 @@ func TestNewJSON(t *testing.T) { func TestNewJSONNoEmail(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) @@ -319,9 +279,7 @@ func TestNewJSONNoEmail(t *testing.T) { } config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) ac := config.AuthConfigs["https://index.docker.io/v1/"] if ac.Username != "joejoe" || ac.Password != "hello" { @@ -346,9 +304,7 @@ func TestNewJSONNoEmail(t *testing.T) { func TestJSONWithPsFormat(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) @@ -361,9 +317,7 @@ func TestJSONWithPsFormat(t *testing.T) { } config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) if config.PsFormat != `table {{.ID}}\t{{.Label "com.docker.label.cpu"}}` { t.Fatalf("Unknown ps format: %s\n", config.PsFormat) @@ -379,9 +333,7 @@ func TestJSONWithPsFormat(t *testing.T) { func TestJSONWithCredentialStore(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) @@ -394,9 +346,7 @@ func TestJSONWithCredentialStore(t *testing.T) { } config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) if config.CredentialsStore != "crazy-secure-storage" { t.Fatalf("Unknown credential store: %s\n", config.CredentialsStore) @@ -412,9 +362,7 @@ func TestJSONWithCredentialStore(t *testing.T) { func TestJSONWithCredentialHelpers(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) @@ -427,9 +375,7 @@ func TestJSONWithCredentialHelpers(t *testing.T) { } config, err := Load(tmpHome) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) if config.CredentialHelpers == nil { t.Fatal("config.CredentialHelpers was nil") @@ -450,26 +396,18 @@ func TestJSONWithCredentialHelpers(t *testing.T) { } // Save it and make sure it shows up in new form -func saveConfigAndValidateNewFormat(t *testing.T, config *configfile.ConfigFile, homeFolder string) string { - if err := config.Save(); err != nil { - t.Fatalf("Failed to save: %q", err) - } +func saveConfigAndValidateNewFormat(t *testing.T, config *configfile.ConfigFile, configDir string) string { + require.NoError(t, config.Save()) - buf, err := ioutil.ReadFile(filepath.Join(homeFolder, ConfigFileName)) - if err != nil { - t.Fatal(err) - } - if !strings.Contains(string(buf), `"auths":`) { - t.Fatalf("Should have save in new form: %s", string(buf)) - } + buf, err := ioutil.ReadFile(filepath.Join(configDir, ConfigFileName)) + require.NoError(t, err) + assert.Contains(t, string(buf), `"auths":`) return string(buf) } func TestConfigDir(t *testing.T) { tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) if Dir() == tmpHome { @@ -488,9 +426,7 @@ func TestJSONReaderNoFile(t *testing.T) { js := ` { "auths": { "https://index.docker.io/v1/": { "auth": "am9lam9lOmhlbGxv", "email": "user@example.com" } } }` config, err := LoadFromReader(strings.NewReader(js)) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) ac := config.AuthConfigs["https://index.docker.io/v1/"] if ac.Username != "joejoe" || ac.Password != "hello" { @@ -503,9 +439,7 @@ func TestOldJSONReaderNoFile(t *testing.T) { js := `{"https://index.docker.io/v1/":{"auth":"am9lam9lOmhlbGxv","email":"user@example.com"}}` config, err := LegacyLoadFromReader(strings.NewReader(js)) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) ac := config.AuthConfigs["https://index.docker.io/v1/"] if ac.Username != "joejoe" || ac.Password != "hello" { @@ -519,9 +453,7 @@ func TestJSONWithPsFormatNoFile(t *testing.T) { "psFormat": "table {{.ID}}\\t{{.Label \"com.docker.label.cpu\"}}" }` config, err := LoadFromReader(strings.NewReader(js)) - if err != nil { - t.Fatalf("Failed loading on empty json file: %q", err) - } + require.NoError(t, err) if config.PsFormat != `table {{.ID}}\t{{.Label "com.docker.label.cpu"}}` { t.Fatalf("Unknown ps format: %s\n", config.PsFormat) @@ -537,28 +469,19 @@ func TestJSONSaveWithNoFile(t *testing.T) { config, err := LoadFromReader(strings.NewReader(js)) require.NoError(t, err) err = config.Save() - if err == nil { - t.Fatalf("Expected error. File should not have been able to save with no file name.") - } + testutil.ErrorContains(t, err, "with empty filename") tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatalf("Failed to create a temp dir: %q", err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) f, _ := os.OpenFile(fn, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) defer f.Close() - err = config.SaveToWriter(f) - if err != nil { - t.Fatalf("Failed saving to file: %q", err) - } + require.NoError(t, config.SaveToWriter(f)) buf, err := ioutil.ReadFile(filepath.Join(tmpHome, ConfigFileName)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) expConfStr := `{ "auths": { "https://index.docker.io/v1/": { @@ -573,32 +496,23 @@ func TestJSONSaveWithNoFile(t *testing.T) { } func TestLegacyJSONSaveWithNoFile(t *testing.T) { - js := `{"https://index.docker.io/v1/":{"auth":"am9lam9lOmhlbGxv","email":"user@example.com"}}` config, err := LegacyLoadFromReader(strings.NewReader(js)) require.NoError(t, err) err = config.Save() - if err == nil { - t.Fatalf("Expected error. File should not have been able to save with no file name.") - } + testutil.ErrorContains(t, err, "with empty filename") tmpHome, err := ioutil.TempDir("", "config-test") - if err != nil { - t.Fatalf("Failed to create a temp dir: %q", err) - } + require.NoError(t, err) defer os.RemoveAll(tmpHome) fn := filepath.Join(tmpHome, ConfigFileName) f, _ := os.OpenFile(fn, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) defer f.Close() - if err = config.SaveToWriter(f); err != nil { - t.Fatalf("Failed saving to file: %q", err) - } + require.NoError(t, config.SaveToWriter(f)) buf, err := ioutil.ReadFile(filepath.Join(tmpHome, ConfigFileName)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) expConfStr := `{ "auths": { @@ -613,3 +527,22 @@ func TestLegacyJSONSaveWithNoFile(t *testing.T) { t.Fatalf("Should have save in new form: \n%s\n not \n%s", string(buf), expConfStr) } } + +func TestLoadDefaultConfigFile(t *testing.T) { + dir, cleanup := setupConfigDir(t) + defer cleanup() + buffer := new(bytes.Buffer) + + filename := filepath.Join(dir, ConfigFileName) + content := []byte(`{"PsFormat": "format"}`) + err := ioutil.WriteFile(filename, content, 0644) + require.NoError(t, err) + + configFile := LoadDefaultConfigFile(buffer) + credStore := credentials.DetectDefaultStore("") + expected := configfile.NewConfigFile(filename) + expected.CredentialsStore = credStore + expected.PsFormat = "format" + + assert.Equal(t, expected, configFile) +} diff --git a/cli/config/configfile/file_test.go b/cli/config/configfile/file_test.go index ac3271006e..4e24009777 100644 --- a/cli/config/configfile/file_test.go +++ b/cli/config/configfile/file_test.go @@ -6,26 +6,18 @@ import ( "github.com/docker/docker/api/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestEncodeAuth(t *testing.T) { newAuthConfig := &types.AuthConfig{Username: "ken", Password: "test"} authStr := encodeAuth(newAuthConfig) - decAuthConfig := &types.AuthConfig{} + + expected := &types.AuthConfig{} var err error - decAuthConfig.Username, decAuthConfig.Password, err = decodeAuth(authStr) - if err != nil { - t.Fatal(err) - } - if newAuthConfig.Username != decAuthConfig.Username { - t.Fatal("Encode Username doesn't match decoded Username") - } - if newAuthConfig.Password != decAuthConfig.Password { - t.Fatal("Encode Password doesn't match decoded Password") - } - if authStr != "a2VuOnRlc3Q=" { - t.Fatal("AuthString encoding isn't correct.") - } + expected.Username, expected.Password, err = decodeAuth(authStr) + require.NoError(t, err) + assert.Equal(t, expected, newAuthConfig) } func TestProxyConfig(t *testing.T) { @@ -149,3 +141,19 @@ func TestConfigFile(t *testing.T) { assert.Equal(t, configFilename, configFile.Filename) } + +func TestGetAllCredentials(t *testing.T) { + configFile := NewConfigFile("filename") + exampleAuth := types.AuthConfig{ + Username: "user", + Password: "pass", + } + configFile.AuthConfigs["example.com/foo"] = exampleAuth + + authConfigs, err := configFile.GetAllCredentials() + require.NoError(t, err) + + expected := make(map[string]types.AuthConfig) + expected["example.com/foo"] = exampleAuth + assert.Equal(t, expected, authConfigs) +} From 105b21d1abf0bb08c67b123118fe6e1a95379984 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 27 Jun 2017 16:31:38 +0200 Subject: [PATCH 5/5] Rename NewConfigFile to New in configfile package Signed-off-by: Vincent Demeester --- cli/command/cli.go | 1 + cli/command/image/build_session.go | 4 ++-- cli/command/image/pull_test.go | 4 ++-- cli/command/image/push_test.go | 4 ++-- cli/config/config.go | 2 +- cli/config/config_test.go | 2 +- cli/config/configfile/file.go | 4 ++-- cli/config/configfile/file_test.go | 4 ++-- cli/internal/test/cli.go | 2 -- 9 files changed, 13 insertions(+), 14 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index 885859abc9..e9b274f5ec 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -36,6 +36,7 @@ type Cli interface { In() *InStream SetIn(in *InStream) ConfigFile() *configfile.ConfigFile + ServerInfo() ServerInfo } // DockerCli is an instance the docker command line client. diff --git a/cli/command/image/build_session.go b/cli/command/image/build_session.go index c010c0ea0b..86fe95782e 100644 --- a/cli/command/image/build_session.go +++ b/cli/command/image/build_session.go @@ -26,11 +26,11 @@ import ( const clientSessionRemote = "client-session" -func isSessionSupported(dockerCli *command.DockerCli) bool { +func isSessionSupported(dockerCli command.Cli) bool { return dockerCli.ServerInfo().HasExperimental && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.31") } -func trySession(dockerCli *command.DockerCli, contextDir string) (*session.Session, error) { +func trySession(dockerCli command.Cli, contextDir string) (*session.Session, error) { var s *session.Session if isSessionSupported(dockerCli) { sharedKey, err := getBuildSharedKey(contextDir) diff --git a/cli/command/image/pull_test.go b/cli/command/image/pull_test.go index 731c774a1c..690e6222eb 100644 --- a/cli/command/image/pull_test.go +++ b/cli/command/image/pull_test.go @@ -43,7 +43,7 @@ func TestNewPullCommandErrors(t *testing.T) { for _, tc := range testCases { buf := new(bytes.Buffer) cli := test.NewFakeCli(&fakeClient{}, buf) - cli.SetConfigfile(configfile.NewConfigFile("filename")) + cli.SetConfigfile(configfile.New("filename")) cmd := NewPullCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) @@ -68,7 +68,7 @@ func TestNewPullCommandSuccess(t *testing.T) { for _, tc := range testCases { buf := new(bytes.Buffer) cli := test.NewFakeCli(&fakeClient{}, buf) - cli.SetConfigfile(configfile.NewConfigFile("filename")) + cli.SetConfigfile(configfile.New("filename")) cmd := NewPullCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) diff --git a/cli/command/image/push_test.go b/cli/command/image/push_test.go index f02e0407d6..f2c35dee21 100644 --- a/cli/command/image/push_test.go +++ b/cli/command/image/push_test.go @@ -49,7 +49,7 @@ func TestNewPushCommandErrors(t *testing.T) { for _, tc := range testCases { buf := new(bytes.Buffer) cli := test.NewFakeCli(&fakeClient{imagePushFunc: tc.imagePushFunc}, buf) - cli.SetConfigfile(configfile.NewConfigFile("filename")) + cli.SetConfigfile(configfile.New("filename")) cmd := NewPushCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) @@ -74,7 +74,7 @@ func TestNewPushCommandSuccess(t *testing.T) { return ioutil.NopCloser(strings.NewReader("")), nil }, }, buf) - cli.SetConfigfile(configfile.NewConfigFile("filename")) + cli.SetConfigfile(configfile.New("filename")) cmd := NewPushCommand(cli) cmd.SetOutput(ioutil.Discard) cmd.SetArgs(tc.args) diff --git a/cli/config/config.go b/cli/config/config.go index 5e638a519f..90529ebd4c 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -69,7 +69,7 @@ func Load(configDir string) (*configfile.ConfigFile, error) { } filename := filepath.Join(configDir, ConfigFileName) - configFile := configfile.NewConfigFile(filename) + configFile := configfile.New(filename) // Try happy path first - latest config file if _, err := os.Stat(filename); err == nil { diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 1248913c5e..c885d3e724 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -540,7 +540,7 @@ func TestLoadDefaultConfigFile(t *testing.T) { configFile := LoadDefaultConfigFile(buffer) credStore := credentials.DetectDefaultStore("") - expected := configfile.NewConfigFile(filename) + expected := configfile.New(filename) expected.CredentialsStore = credStore expected.PsFormat = "format" diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index d599cc5aa9..babd63693b 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -54,8 +54,8 @@ type ProxyConfig struct { FTPProxy string `json:"ftpProxy,omitempty"` } -// NewConfigFile initializes an empty configuration file for the given filename 'fn' -func NewConfigFile(fn string) *ConfigFile { +// New initializes an empty configuration file for the given filename 'fn' +func New(fn string) *ConfigFile { return &ConfigFile{ AuthConfigs: make(map[string]types.AuthConfig), HTTPHeaders: make(map[string]string), diff --git a/cli/config/configfile/file_test.go b/cli/config/configfile/file_test.go index 4e24009777..f2a61b1794 100644 --- a/cli/config/configfile/file_test.go +++ b/cli/config/configfile/file_test.go @@ -137,13 +137,13 @@ func TestProxyConfigPerHost(t *testing.T) { func TestConfigFile(t *testing.T) { configFilename := "configFilename" - configFile := NewConfigFile(configFilename) + configFile := New(configFilename) assert.Equal(t, configFilename, configFile.Filename) } func TestGetAllCredentials(t *testing.T) { - configFile := NewConfigFile("filename") + configFile := New("filename") exampleAuth := types.AuthConfig{ Username: "user", Password: "pass", diff --git a/cli/internal/test/cli.go b/cli/internal/test/cli.go index fa77a86a31..9565ae8e2d 100644 --- a/cli/internal/test/cli.go +++ b/cli/internal/test/cli.go @@ -7,7 +7,6 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/config/configfile" - "github.com/docker/cli/cli/config/credentials" "github.com/docker/docker/client" ) @@ -19,7 +18,6 @@ type FakeCli struct { out *command.OutStream err io.Writer in *command.InStream - store credentials.Store } // NewFakeCli returns a Cli backed by the fakeCli