From 24a66d3c9f586b17f8855eb6a29f44eb56fdb6bf Mon Sep 17 00:00:00 2001 From: Jan Ulferts Date: Mon, 25 Feb 2019 11:34:26 +0100 Subject: [PATCH 1/5] added NewClient() and testing --- dcos/client.go | 43 +++++++++++++++++++++++++++++++++++------ dcos/client_test.go | 42 +++++++++++++++++++++++----------------- dcos/config.go | 16 +++++++++++++++ dcos/config_test.go | 16 ++++++++++++++- dcos/httpclient.go | 7 +++++-- dcos/httpclient_test.go | 17 ++++++++++------ 6 files changed, 108 insertions(+), 33 deletions(-) diff --git a/dcos/client.go b/dcos/client.go index d45d3a8..c1f9f56 100644 --- a/dcos/client.go +++ b/dcos/client.go @@ -1,16 +1,47 @@ package dcos -import "net/http" +import ( + "fmt" + "net/http" +) +// Client type Client struct { - HttpClient *http.Client + HTTPClient *http.Client + Config *Config + UserAgent string +} - baseURL string +// NewClient returns a Client which will detect its Config through the well known +// process and use a http.Client with DC/OS authentication. +func NewClient() (*Client, error) { + return NewClientWithOptions(nil, nil) } -func NewClient(httpClient *http.Client) (*Client, error) { +// NewClientWithOptions creates a Client from a given *http.Client and *Config +// if nil is given for one or both options defaults will be taken +// +// dcosClient, err := NewClientWithOptions(nil,nil) +// +func NewClientWithOptions(httpClient *http.Client, config *Config) (*Client, error) { + if config == nil { + currentConfig, err := NewConfigManager(DefaultConfigManagerOpts).Current() + if err != nil { + return nil, err + } + config = currentConfig + } + if httpClient == nil { - httpClient = http.DefaultClient + hc, err := NewHTTPClient(config) + if err != nil { + return nil, err + } + httpClient = hc } - return &Client{HttpClient: httpClient}, nil + return &Client{ + HTTPClient: httpClient, + Config: config, + UserAgent: fmt.Sprintf("%s(%s)", ClientName, Version), + }, nil } diff --git a/dcos/client_test.go b/dcos/client_test.go index ceae41e..4f5247e 100644 --- a/dcos/client_test.go +++ b/dcos/client_test.go @@ -2,28 +2,34 @@ package dcos import ( "net/http" + "os" + "path/filepath" "testing" + "time" + + homedir "github.com/mitchellh/go-homedir" + "github.com/stretchr/testify/require" ) -func TestClientNew(t *testing.T) { - baseClient := &http.Client{} - c, err := NewClient(baseClient) - if err != nil { - t.Fatalf("NewClient returned unexpected error: %s", err) - } +func TestNewClient(t *testing.T) { + homedir.DisableCache = true + wd, err := os.Getwd() + testdir := filepath.Join(wd, "testdata", "config", "single_config_attached") + require.NoError(t, err) + os.Setenv("HOME", testdir) + c, err := NewClient() - if c.HttpClient != baseClient { - t.Errorf("client.HttpClient wrong. got=%+v, want=%+v", - c.HttpClient, baseClient) - } + require.NoErrorf(t, err, "NewClient returned unexpected error: %s - %s", err, testdir) + require.IsTypef(t, &DefaultTransport{}, c.HTTPClient.Transport, "HTTPClient.Transport type different") +} - c, err = NewClient(nil) - if err != nil { - t.Fatalf("NewClient returned unexpected error: %s", err) - } +func TestNewClientWithOptions(t *testing.T) { + timeout := 7 * time.Second + baseClient := &http.Client{Timeout: timeout} + config := NewConfig(nil) + config.SetACSToken("test") + c, err := NewClientWithOptions(baseClient, config) - if c.HttpClient != http.DefaultClient { - t.Errorf("client.HttpClient not http.Defaultclient. got=%+v", - c.HttpClient) - } + require.NoErrorf(t, err, "NewClientWithOptions returned unexpected error: %s", err) + require.Equal(t, baseClient, c.HTTPClient, "NewClientWithOptions should leave HTTPClient unchanged") } diff --git a/dcos/config.go b/dcos/config.go index ac54d35..2e9d332 100644 --- a/dcos/config.go +++ b/dcos/config.go @@ -12,6 +12,7 @@ import ( "strings" "time" + homedir "github.com/mitchellh/go-homedir" toml "github.com/pelletier/go-toml" "github.com/spf13/afero" "github.com/spf13/cast" @@ -30,6 +31,7 @@ const ( configKeyMesosMasterURL = "core.mesos_master_url" configKeyPromptLogin = "core.prompt_login" configKeyClusterName = "cluster.name" + dcosDefaultFolder = "~/.dcos" ) // Environment variables for the DC/OS configuration. @@ -445,6 +447,20 @@ type ConfigManagerOpts struct { Dir string } +func ExpandHomeDir() string { + dir, err := homedir.Expand(dcosDefaultFolder) + if err != nil { + return dcosDefaultFolder + } + return dir +} + +var DefaultConfigManagerOpts = &ConfigManagerOpts{ + Fs: afero.NewOsFs(), + Dir: ExpandHomeDir(), + EnvLookup: os.LookupEnv, +} + // ConfigManager is able to retrieve, create, and delete configs. type ConfigManager struct { fs afero.Fs diff --git a/dcos/config_test.go b/dcos/config_test.go index 775da19..6a27d8d 100644 --- a/dcos/config_test.go +++ b/dcos/config_test.go @@ -9,11 +9,18 @@ import ( "testing" "time" - "github.com/pelletier/go-toml" + homedir "github.com/mitchellh/go-homedir" + toml "github.com/pelletier/go-toml" "github.com/spf13/afero" "github.com/stretchr/testify/require" ) +func TestMain(m *testing.M) { + homedir.DisableCache = true + // call flag.Parse() here if TestMain uses flags + os.Exit(m.Run()) +} + func TestConfigGetters(t *testing.T) { store := NewConfigStore(nil) @@ -533,3 +540,10 @@ func TestConfigAttach(t *testing.T) { require.NoError(t, manager.Attach(conf)) require.True(t, manager.fileExists(attachedFilePath)) } + +func TestexpandHomedir(t *testing.T) { + os.Setenv("HOME", "/home/testuser") + dir := ExpandHomeDir() + + require.Equal(t, dir, "/home/testuser/.dcos", "wrong return") +} diff --git a/dcos/httpclient.go b/dcos/httpclient.go index 9787150..f29a90c 100644 --- a/dcos/httpclient.go +++ b/dcos/httpclient.go @@ -55,7 +55,10 @@ func cloneRequest(req *http.Request) *http.Request { } // NewHTTPClient provides a http.Client able to communicate to dcos in an authenticated way -func NewHTTPClient(config *Config) *http.Client { +func NewHTTPClient(config *Config) (*http.Client, error) { + if config == nil { + return nil, fmt.Errorf("Config should not be nil") + } client := &http.Client{} client.Transport = &http.Transport{ @@ -76,7 +79,7 @@ func NewHTTPClient(config *Config) *http.Client { MaxIdleConnsPerHost: DefaultHTTPClientMaxIdleConnsPerHost, } - return AddTransportHTTPClient(client, config) + return AddTransportHTTPClient(client, config), nil } // AddTransportHTTPClient adds dcos.DefaultTransport to http.Client to add dcos authentication diff --git a/dcos/httpclient_test.go b/dcos/httpclient_test.go index 207cc5a..74ec2ec 100644 --- a/dcos/httpclient_test.go +++ b/dcos/httpclient_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDefaultTransportBaseNil(t *testing.T) { @@ -23,7 +24,7 @@ func TestDefaultTransportBase(t *testing.T) { } rt := c.base() - assert.IsType(t, &DefaultTransport{}, rt) + require.IsType(t, &DefaultTransport{}, rt) } func TestDefaultHTTPClientAuth(t *testing.T) { @@ -42,13 +43,17 @@ func TestDefaultHTTPClientAuth(t *testing.T) { })) defer s.Close() - c := NewHTTPClient(config) + _, err := NewHTTPClient(nil) + require.Error(t, err) + + c, err := NewHTTPClient(config) + require.NoError(t, err) resp, err := c.Get(s.URL) - assert.NoError(t, err) - assert.Equal(t, 200, resp.StatusCode, "using the dcos.NewHTTPClient should respond with 200") + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode, "using the dcos.NewHTTPClient should respond with 200") respDflt, err := http.DefaultClient.Get(s.URL) - assert.NoError(t, err) - assert.Equal(t, 401, respDflt.StatusCode, "expect a forbidden state with http.DefaultClient") + require.NoError(t, err) + require.Equal(t, 401, respDflt.StatusCode, "expect a forbidden state with http.DefaultClient") } From aba4292599033f8e950942401beea9f48e4e05f1 Mon Sep 17 00:00:00 2001 From: Jan Ulferts Date: Mon, 25 Feb 2019 11:57:28 +0100 Subject: [PATCH 2/5] fixed typo in test name --- dcos/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dcos/config_test.go b/dcos/config_test.go index 6a27d8d..b3a027a 100644 --- a/dcos/config_test.go +++ b/dcos/config_test.go @@ -541,7 +541,7 @@ func TestConfigAttach(t *testing.T) { require.True(t, manager.fileExists(attachedFilePath)) } -func TestexpandHomedir(t *testing.T) { +func TestExpandHomedir(t *testing.T) { os.Setenv("HOME", "/home/testuser") dir := ExpandHomeDir() From e8cfdedbfdadac54363937ef000d21f7b9ff0d4b Mon Sep 17 00:00:00 2001 From: Jan Ulferts Date: Mon, 25 Feb 2019 14:51:59 +0100 Subject: [PATCH 3/5] deleted DefaultConfigManagerOpts NewConfigManager handles defaults --- dcos/config.go | 10 ++++------ dcos/config_test.go | 7 +------ 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/dcos/config.go b/dcos/config.go index 2e9d332..d487298 100644 --- a/dcos/config.go +++ b/dcos/config.go @@ -455,12 +455,6 @@ func ExpandHomeDir() string { return dir } -var DefaultConfigManagerOpts = &ConfigManagerOpts{ - Fs: afero.NewOsFs(), - Dir: ExpandHomeDir(), - EnvLookup: os.LookupEnv, -} - // ConfigManager is able to retrieve, create, and delete configs. type ConfigManager struct { fs afero.Fs @@ -482,6 +476,10 @@ func NewConfigManager(opts *ConfigManagerOpts) *ConfigManager { opts.EnvLookup = os.LookupEnv } + if opts.Dir == "" { + opts.Dir = ExpandHomeDir() + } + return &ConfigManager{ fs: opts.Fs, dir: opts.Dir, diff --git a/dcos/config_test.go b/dcos/config_test.go index b3a027a..80afefc 100644 --- a/dcos/config_test.go +++ b/dcos/config_test.go @@ -15,12 +15,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestMain(m *testing.M) { - homedir.DisableCache = true - // call flag.Parse() here if TestMain uses flags - os.Exit(m.Run()) -} - func TestConfigGetters(t *testing.T) { store := NewConfigStore(nil) @@ -542,6 +536,7 @@ func TestConfigAttach(t *testing.T) { } func TestExpandHomedir(t *testing.T) { + homedir.DisableCache = true os.Setenv("HOME", "/home/testuser") dir := ExpandHomeDir() From 9313e4a8038dbe1005fb95aba348a24dc996fc66 Mon Sep 17 00:00:00 2001 From: Jan Ulferts Date: Mon, 25 Feb 2019 14:54:12 +0100 Subject: [PATCH 4/5] changed NewClientWithOptions to require httpClient and config --- dcos/client.go | 37 ++++++++++++++++++------------------- dcos/client_test.go | 12 ++++++++++-- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/dcos/client.go b/dcos/client.go index c1f9f56..dcd5ecd 100644 --- a/dcos/client.go +++ b/dcos/client.go @@ -12,33 +12,32 @@ type Client struct { UserAgent string } -// NewClient returns a Client which will detect its Config through the well known -// process and use a http.Client with DC/OS authentication. +// NewClient returns a Client which will detect its Config through the well +// known process and use a default http.Client with DC/OS authentication. func NewClient() (*Client, error) { - return NewClientWithOptions(nil, nil) + config, err := NewConfigManager(nil).Current() + if err != nil { + return nil, err + } + + httpClient, err := NewHTTPClient(config) + if err != nil { + return nil, err + } + + return NewClientWithOptions(httpClient, config) } // NewClientWithOptions creates a Client from a given *http.Client and *Config -// if nil is given for one or both options defaults will be taken -// -// dcosClient, err := NewClientWithOptions(nil,nil) -// func NewClientWithOptions(httpClient *http.Client, config *Config) (*Client, error) { - if config == nil { - currentConfig, err := NewConfigManager(DefaultConfigManagerOpts).Current() - if err != nil { - return nil, err - } - config = currentConfig + if httpClient == nil { + return nil, fmt.Errorf("httpClient cannot be nil") } - if httpClient == nil { - hc, err := NewHTTPClient(config) - if err != nil { - return nil, err - } - httpClient = hc + if config == nil { + return nil, fmt.Errorf("config cannot be nil") } + return &Client{ HTTPClient: httpClient, Config: config, diff --git a/dcos/client_test.go b/dcos/client_test.go index 4f5247e..56ea03f 100644 --- a/dcos/client_test.go +++ b/dcos/client_test.go @@ -14,12 +14,14 @@ import ( func TestNewClient(t *testing.T) { homedir.DisableCache = true wd, err := os.Getwd() - testdir := filepath.Join(wd, "testdata", "config", "single_config_attached") require.NoError(t, err) + testdir := filepath.Join(wd, "testdata", "config", "single_config_attached") os.Setenv("HOME", testdir) + c, err := NewClient() - require.NoErrorf(t, err, "NewClient returned unexpected error: %s - %s", err, testdir) + require.NoErrorf(t, err, "NewClient returned unexpected error: %s - testdir: %s", err, testdir) + require.Equal(t, "single_config_attached", c.Config.Name()) require.IsTypef(t, &DefaultTransport{}, c.HTTPClient.Transport, "HTTPClient.Transport type different") } @@ -32,4 +34,10 @@ func TestNewClientWithOptions(t *testing.T) { require.NoErrorf(t, err, "NewClientWithOptions returned unexpected error: %s", err) require.Equal(t, baseClient, c.HTTPClient, "NewClientWithOptions should leave HTTPClient unchanged") + + _, err = NewClientWithOptions(nil, config) + require.Error(t, err) + + _, err = NewClientWithOptions(baseClient, nil) + require.Error(t, err) } From c4604ec9a04b18d96421d6f03d690c4e98283d2b Mon Sep 17 00:00:00 2001 From: Jan Ulferts Date: Thu, 28 Feb 2019 12:13:07 +0100 Subject: [PATCH 5/5] made private ExpandHomeDir to expandHomeDir --- dcos/config.go | 4 ++-- dcos/config_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dcos/config.go b/dcos/config.go index d487298..d888c31 100644 --- a/dcos/config.go +++ b/dcos/config.go @@ -447,7 +447,7 @@ type ConfigManagerOpts struct { Dir string } -func ExpandHomeDir() string { +func expandHomeDir() string { dir, err := homedir.Expand(dcosDefaultFolder) if err != nil { return dcosDefaultFolder @@ -477,7 +477,7 @@ func NewConfigManager(opts *ConfigManagerOpts) *ConfigManager { } if opts.Dir == "" { - opts.Dir = ExpandHomeDir() + opts.Dir = expandHomeDir() } return &ConfigManager{ diff --git a/dcos/config_test.go b/dcos/config_test.go index 80afefc..38e2c3f 100644 --- a/dcos/config_test.go +++ b/dcos/config_test.go @@ -538,7 +538,7 @@ func TestConfigAttach(t *testing.T) { func TestExpandHomedir(t *testing.T) { homedir.DisableCache = true os.Setenv("HOME", "/home/testuser") - dir := ExpandHomeDir() + dir := expandHomeDir() require.Equal(t, dir, "/home/testuser/.dcos", "wrong return") }