Cleanup client/ interface

- Remove ParseLogDetails, this is not part of the client. Moved to docker/cli
- Deprecate ParseHost and replace with ParseHostURL
- Deprecate redundant IsErr helpers

Signed-off-by: Daniel Nephin <dnephin@docker.com>

client/client.go

@@ -1,10 +1,6 @@
 /*
 Package client is a Go client for the Docker Engine API.
 
-The "docker" command uses this package to communicate with the daemon. It can also
-be used by your own Go applications to do anything the command-line interface does
-- running containers, pulling images, managing swarms, etc.
-
 For more information about the Engine API, see the documentation:
 https://docs.docker.com/engine/reference/api/
 
@@ -160,7 +156,7 @@ func NewEnvClient() (*Client, error) {
 // highly recommended that you set a version or your client may break if the
 // server is upgraded.
 func NewClient(host string, version string, client *http.Client, httpHeaders map[string]string) (*Client, error) {
-	proto, addr, basePath, err := ParseHost(host)
+	hostURL, err := ParseHostURL(host)
 	if err != nil {
 		return nil, err
 	}
@@ -171,7 +167,7 @@ func NewClient(host string, version string, client *http.Client, httpHeaders map
 		}
 	} else {
 		transport := new(http.Transport)
-		sockets.ConfigureTransport(transport, proto, addr)
+		sockets.ConfigureTransport(transport, hostURL.Scheme, hostURL.Host)
 		client = &http.Client{
 			Transport:     transport,
 			CheckRedirect: CheckRedirect,
@@ -189,28 +185,24 @@ func NewClient(host string, version string, client *http.Client, httpHeaders map
 		scheme = "https"
 	}
 
+	// TODO: store URL instead of proto/addr/basePath
 	return &Client{
 		scheme:            scheme,
 		host:              host,
-		proto:             proto,
-		addr:              addr,
-		basePath:          basePath,
+		proto:             hostURL.Scheme,
+		addr:              hostURL.Host,
+		basePath:          hostURL.Path,
 		client:            client,
 		version:           version,
 		customHTTPHeaders: httpHeaders,
 	}, nil
 }
 
-// Close ensures that transport.Client is closed
-// especially needed while using NewClient with *http.Client = nil
-// for example
-// client.NewClient("unix:///var/run/docker.sock", nil, "v1.18", map[string]string{"User-Agent": "engine-api-cli-1.0"})
+// Close the transport used by the client
 func (cli *Client) Close() error {
-
 	if t, ok := cli.client.Transport.(*http.Transport); ok {
 		t.CloseIdleConnections()
 	}
-
 	return nil
 }
 
@@ -234,23 +226,20 @@ func (cli *Client) getAPIPath(p string, query url.Values) string {
 	return u.String()
 }
 
-// ClientVersion returns the version string associated with this
-// instance of the Client. Note that this value can be changed
-// via the DOCKER_API_VERSION env var.
-// This operation doesn't acquire a mutex.
+// ClientVersion returns the API version used by this client.
 func (cli *Client) ClientVersion() string {
 	return cli.version
 }
 
-// NegotiateAPIVersion updates the version string associated with this
-// instance of the Client to match the latest version the server supports
+// NegotiateAPIVersion queries the API and updates the version to match the
+// API version. Any errors are silently ignored.
 func (cli *Client) NegotiateAPIVersion(ctx context.Context) {
 	ping, _ := cli.Ping(ctx)
 	cli.NegotiateAPIVersionPing(ping)
 }
 
-// NegotiateAPIVersionPing updates the version string associated with this
-// instance of the Client to match the latest version the server supports
+// NegotiateAPIVersionPing updates the client version to match the Ping.APIVersion
+// if the ping version is less than the default version.
 func (cli *Client) NegotiateAPIVersionPing(p types.Ping) {
 	if cli.manualOverride {
 		return
@@ -272,17 +261,28 @@ func (cli *Client) NegotiateAPIVersionPing(p types.Ping) {
 	}
 }
 
-// DaemonHost returns the host associated with this instance of the Client.
-// This operation doesn't acquire a mutex.
+// DaemonHost returns the host address used by the client
 func (cli *Client) DaemonHost() string {
 	return cli.host
 }
 
-// ParseHost verifies that the given host strings is valid.
+// ParseHost parses a url string, validates the strings is a host url, and returns
+// the parsed host as: protocol, address, and base path
+// Deprecated: use ParseHostURL
 func ParseHost(host string) (string, string, string, error) {
+	hostURL, err := ParseHostURL(host)
+	if err != nil {
+		return "", "", "", err
+	}
+	return hostURL.Scheme, hostURL.Host, hostURL.Path, nil
+}
+
+// ParseHostURL parses a url string, validates the string is a host url, and
+// returns the parsed URL
+func ParseHostURL(host string) (*url.URL, error) {
 	protoAddrParts := strings.SplitN(host, "://", 2)
 	if len(protoAddrParts) == 1 {
-		return "", "", "", fmt.Errorf("unable to parse docker host `%s`", host)
+		return nil, fmt.Errorf("unable to parse docker host `%s`", host)
 	}
 
 	var basePath string
@@ -290,16 +290,19 @@ func ParseHost(host string) (string, string, string, error) {
 	if proto == "tcp" {
 		parsed, err := url.Parse("tcp://" + addr)
 		if err != nil {
-			return "", "", "", err
+			return nil, err
 		}
 		addr = parsed.Host
 		basePath = parsed.Path
 	}
-	return proto, addr, basePath, nil
+	return &url.URL{
+		Scheme: proto,
+		Host:   addr,
+		Path:   basePath,
+	}, nil
 }
 
-// CustomHTTPHeaders returns the custom http headers associated with this
-// instance of the Client. This operation doesn't acquire a mutex.
+// CustomHTTPHeaders returns the custom http headers stored by the client.
 func (cli *Client) CustomHTTPHeaders() map[string]string {
 	m := make(map[string]string)
 	for k, v := range cli.customHTTPHeaders {
@@ -308,8 +311,7 @@ func (cli *Client) CustomHTTPHeaders() map[string]string {
 	return m
 }
 
-// SetCustomHTTPHeaders updates the custom http headers associated with this
-// instance of the Client. This operation doesn't acquire a mutex.
+// SetCustomHTTPHeaders that will be set on every HTTP request made by the client.
 func (cli *Client) SetCustomHTTPHeaders(headers map[string]string) {
 	cli.customHTTPHeaders = headers
 }

client/client_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/internal/testutil"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -152,7 +153,6 @@ func TestParseHost(t *testing.T) {
 
 	for _, cs := range cases {
 		p, a, b, e := ParseHost(cs.host)
-		// if we expected an error to be returned...
 		if cs.err {
 			assert.Error(t, e)
 		}
@@ -162,6 +162,43 @@ func TestParseHost(t *testing.T) {
 	}
 }
 
+func TestParseHostURL(t *testing.T) {
+	testcases := []struct {
+		host        string
+		expected    *url.URL
+		expectedErr string
+	}{
+		{
+			host:        "",
+			expectedErr: "unable to parse docker host",
+		},
+		{
+			host:        "foobar",
+			expectedErr: "unable to parse docker host",
+		},
+		{
+			host:     "foo://bar",
+			expected: &url.URL{Scheme: "foo", Host: "bar"},
+		},
+		{
+			host:     "tcp://localhost:2476",
+			expected: &url.URL{Scheme: "tcp", Host: "localhost:2476"},
+		},
+		{
+			host:     "tcp://localhost:2476/path",
+			expected: &url.URL{Scheme: "tcp", Host: "localhost:2476", Path: "/path"},
+		},
+	}
+
+	for _, testcase := range testcases {
+		actual, err := ParseHostURL(testcase.host)
+		if testcase.expectedErr != "" {
+			testutil.ErrorContains(t, err, testcase.expectedErr)
+		}
+		assert.Equal(t, testcase.expected, actual)
+	}
+}
+
 func TestNewEnvClientSetsDefaultVersion(t *testing.T) {
 	env := envToMap()
 	defer mapToEnv(env)

client/errors.go

@@ -36,8 +36,8 @@ type notFound interface {
 	NotFound() bool // Is the error a NotFound error
 }
 
-// IsErrNotFound returns true if the error is caused with an
-// object (image, container, network, volume, …) is not found in the docker host.
+// IsErrNotFound returns true if the error is a NotFound error, which is returned
+// by the API when some object is not found.
 func IsErrNotFound(err error) bool {
 	te, ok := err.(notFound)
 	return ok && te.NotFound()
@@ -60,6 +60,8 @@ func (e imageNotFoundError) Error() string {
 
 // IsErrImageNotFound returns true if the error is caused
 // when an image is not found in the docker host.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrImageNotFound(err error) bool {
 	return IsErrNotFound(err)
 }
@@ -81,6 +83,8 @@ func (e containerNotFoundError) Error() string {
 
 // IsErrContainerNotFound returns true if the error is caused
 // when a container is not found in the docker host.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrContainerNotFound(err error) bool {
 	return IsErrNotFound(err)
 }
@@ -102,6 +106,8 @@ func (e networkNotFoundError) Error() string {
 
 // IsErrNetworkNotFound returns true if the error is caused
 // when a network is not found in the docker host.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrNetworkNotFound(err error) bool {
 	return IsErrNotFound(err)
 }
@@ -123,6 +129,8 @@ func (e volumeNotFoundError) Error() string {
 
 // IsErrVolumeNotFound returns true if the error is caused
 // when a volume is not found in the docker host.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrVolumeNotFound(err error) bool {
 	return IsErrNotFound(err)
 }
@@ -161,6 +169,8 @@ func (e nodeNotFoundError) NotFound() bool {
 
 // IsErrNodeNotFound returns true if the error is caused
 // when a node is not found.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrNodeNotFound(err error) bool {
 	_, ok := err.(nodeNotFoundError)
 	return ok
@@ -183,6 +193,8 @@ func (e serviceNotFoundError) NotFound() bool {
 
 // IsErrServiceNotFound returns true if the error is caused
 // when a service is not found.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrServiceNotFound(err error) bool {
 	_, ok := err.(serviceNotFoundError)
 	return ok
@@ -205,6 +217,8 @@ func (e taskNotFoundError) NotFound() bool {
 
 // IsErrTaskNotFound returns true if the error is caused
 // when a task is not found.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrTaskNotFound(err error) bool {
 	_, ok := err.(taskNotFoundError)
 	return ok
@@ -251,6 +265,8 @@ func (e secretNotFoundError) NotFound() bool {
 
 // IsErrSecretNotFound returns true if the error is caused
 // when a secret is not found.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrSecretNotFound(err error) bool {
 	_, ok := err.(secretNotFoundError)
 	return ok
@@ -273,6 +289,8 @@ func (e configNotFoundError) NotFound() bool {
 
 // IsErrConfigNotFound returns true if the error is caused
 // when a config is not found.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrConfigNotFound(err error) bool {
 	_, ok := err.(configNotFoundError)
 	return ok
@@ -295,6 +313,8 @@ func (e pluginNotFoundError) Error() string {
 
 // IsErrPluginNotFound returns true if the error is caused
 // when a plugin is not found in the docker host.
+//
+// Deprecated: Use IsErrNotFound
 func IsErrPluginNotFound(err error) bool {
 	return IsErrNotFound(err)
 }

client/parse_logs.go

@@ -1,41 +0,0 @@
-package client
-
-// parse_logs.go contains utility helpers for getting information out of docker
-// log lines. really, it only contains ParseDetails right now. maybe in the
-// future there will be some desire to parse log messages back into a struct?
-// that would go here if we did
-
-import (
-	"net/url"
-	"strings"
-
-	"github.com/pkg/errors"
-)
-
-// ParseLogDetails takes a details string of key value pairs in the form
-// "k=v,l=w", where the keys and values are url query escaped, and each pair
-// is separated by a comma, returns a map. returns an error if the details
-// string is not in a valid format
-// the exact form of details encoding is implemented in
-// api/server/httputils/write_log_stream.go
-func ParseLogDetails(details string) (map[string]string, error) {
-	pairs := strings.Split(details, ",")
-	detailsMap := make(map[string]string, len(pairs))
-	for _, pair := range pairs {
-		p := strings.SplitN(pair, "=", 2)
-		// if there is no equals sign, we will only get 1 part back
-		if len(p) != 2 {
-			return nil, errors.New("invalid details format")
-		}
-		k, err := url.QueryUnescape(p[0])
-		if err != nil {
-			return nil, err
-		}
-		v, err := url.QueryUnescape(p[1])
-		if err != nil {
-			return nil, err
-		}
-		detailsMap[k] = v
-	}
-	return detailsMap, nil
-}

client/parse_logs_test.go

@@ -1,36 +0,0 @@
-package client
-
-import (
-	"reflect"
-	"testing"
-
-	"github.com/pkg/errors"
-)
-
-func TestParseLogDetails(t *testing.T) {
-	testCases := []struct {
-		line     string
-		expected map[string]string
-		err      error
-	}{
-		{"key=value", map[string]string{"key": "value"}, nil},
-		{"key1=value1,key2=value2", map[string]string{"key1": "value1", "key2": "value2"}, nil},
-		{"key+with+spaces=value%3Dequals,asdf%2C=", map[string]string{"key with spaces": "value=equals", "asdf,": ""}, nil},
-		{"key=,=nothing", map[string]string{"key": "", "": "nothing"}, nil},
-		{"=", map[string]string{"": ""}, nil},
-		{"errors", nil, errors.New("invalid details format")},
-	}
-	for _, tc := range testCases {
-		tc := tc // capture range variable
-		t.Run(tc.line, func(t *testing.T) {
-			t.Parallel()
-			res, err := ParseLogDetails(tc.line)
-			if err != nil && (err.Error() != tc.err.Error()) {
-				t.Fatalf("unexpected error parsing logs:\nExpected:\n\t%v\nActual:\n\t%v", tc.err, err)
-			}
-			if !reflect.DeepEqual(tc.expected, res) {
-				t.Errorf("result does not match expected:\nExpected:\n\t%#v\nActual:\n\t%#v", tc.expected, res)
-			}
-		})
-	}
-}