opts: use stdlib errors and touch-up some errors

- remove uses of github.com/pkg/errors
- slight improvement on handling parsing errors
- add some test-cases

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

opts/duration.go

@@ -1,9 +1,8 @@
 package opts
 
 import (
+	"errors"
 	"time"
-
-	"github.com/pkg/errors"
 )
 
 // PositiveDurationOpt is an option type for time.Duration that uses a pointer.
@@ -20,7 +19,7 @@ func (d *PositiveDurationOpt) Set(s string) error {
 		return err
 	}
 	if *d.DurationOpt.value < 0 {
-		return errors.Errorf("duration cannot be negative")
+		return errors.New("duration cannot be negative")
 	}
 	return nil
 }

opts/env.go

@@ -1,10 +1,9 @@
 package opts
 
 import (
+	"errors"
 	"os"
 	"strings"
-
-	"github.com/pkg/errors"
 )
 
 // ValidateEnv validates an environment variable and returns it.

opts/gpus.go

@@ -2,12 +2,12 @@ package opts
 
 import (
 	"encoding/csv"
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
 
 	"github.com/docker/docker/api/types/container"
-	"github.com/pkg/errors"
 )
 
 // GpuOpts is a Value type for parsing mounts
@@ -21,7 +21,11 @@ func parseCount(s string) (int, error) {
 	}
 	i, err := strconv.Atoi(s)
 	if err != nil {
-		return 0, errors.Wrap(err, "count must be an integer")
+		var numErr *strconv.NumError
+		if errors.As(err, &numErr) {
+			err = numErr.Err
+		}
+		return 0, fmt.Errorf(`invalid count (%s): value must be either "all" or an integer: %w`, s, err)
 	}
 	return i, nil
 }
@@ -72,7 +76,7 @@ func (o *GpuOpts) Set(value string) error {
 			r := csv.NewReader(strings.NewReader(val))
 			optFields, err := r.Read()
 			if err != nil {
-				return errors.Wrap(err, "failed to read gpu options")
+				return fmt.Errorf("failed to read gpu options: %w", err)
 			}
 			req.Options = ConvertKVStringsToMap(optFields)
 		default:

opts/gpus_test.go

@@ -16,7 +16,7 @@ func TestGpusOptAll(t *testing.T) {
 		"count=-1",
 	} {
 		var gpus GpuOpts
-		gpus.Set(testcase)
+		assert.Check(t, gpus.Set(testcase))
 		gpuReqs := gpus.Value()
 		assert.Assert(t, is.Len(gpuReqs, 1))
 		assert.Check(t, is.DeepEqual(gpuReqs[0], container.DeviceRequest{
@@ -27,15 +27,21 @@ func TestGpusOptAll(t *testing.T) {
 	}
 }
 
+func TestGpusOptInvalidCount(t *testing.T) {
+	var gpus GpuOpts
+	err := gpus.Set(`count=invalid-integer`)
+	assert.Error(t, err, `invalid count (invalid-integer): value must be either "all" or an integer: invalid syntax`)
+}
+
 func TestGpusOpts(t *testing.T) {
 	for _, testcase := range []string{
-		"driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\"",
-		"1,driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\"",
-		"count=1,driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\"",
-		"driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\",count=1",
+		`driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux"`,
+		`1,driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux"`,
+		`count=1,driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux"`,
+		`driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux",count=1`,
 	} {
 		var gpus GpuOpts
-		gpus.Set(testcase)
+		assert.Check(t, gpus.Set(testcase))
 		gpuReqs := gpus.Value()
 		assert.Assert(t, is.Len(gpuReqs, 1))
 		assert.Check(t, is.DeepEqual(gpuReqs[0], container.DeviceRequest{

opts/mount.go

@@ -135,8 +135,7 @@ func (m *MountOpt) Set(value string) error {
 				// TODO: implicitly set propagation and error if the user specifies a propagation in a future refactor/UX polish pass
 				// https://github.com/docker/cli/pull/4316#discussion_r1341974730
 			default:
-				return fmt.Errorf("invalid value for %s: %s (must be \"enabled\", \"disabled\", \"writable\", or \"readonly\")",
-					key, val)
+				return fmt.Errorf(`invalid value for %s: %s (must be "enabled", "disabled", "writable", or "readonly")`, key, val)
 			}
 		case "volume-subpath":
 			volumeOptions().Subpath = val

opts/network.go

@@ -89,7 +89,11 @@ func (n *NetworkOpt) Set(value string) error { //nolint:gocyclo
 			case gwPriorityOpt:
 				netOpt.GwPriority, err = strconv.Atoi(val)
 				if err != nil {
-					return fmt.Errorf("invalid gw-priority: %w", err)
+					var numErr *strconv.NumError
+					if errors.As(err, &numErr) {
+						err = numErr.Err
+					}
+					return fmt.Errorf("invalid gw-priority (%s): %w", val, err)
 				}
 			default:
 				return errors.New("invalid field key " + key)

opts/network_test.go

@@ -149,6 +149,10 @@ func TestNetworkOptAdvancedSyntaxInvalid(t *testing.T) {
 			value:         "driver-opt=field1=value1,driver-opt=field2=value2",
 			expectedError: "network name/id is not specified",
 		},
+		{
+			value:         "gw-priority=invalid-integer",
+			expectedError: "invalid gw-priority (invalid-integer): invalid syntax",
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.value, func(t *testing.T) {

opts/opts.go

@@ -1,6 +1,7 @@
 package opts
 
 import (
+	"errors"
 	"fmt"
 	"math/big"
 	"net"
@@ -9,8 +10,7 @@ import (
 	"strings"
 
 	"github.com/docker/docker/api/types/filters"
-	units "github.com/docker/go-units"
-	"github.com/pkg/errors"
+	"github.com/docker/go-units"
 )
 
 var (

opts/opts_test.go

@@ -136,10 +136,10 @@ func TestListOptsWithoutValidator(t *testing.T) {
 		t.Errorf("%d != 3", o.Len())
 	}
 	if !o.Get("bar") {
-		t.Error("o.Get(\"bar\") == false")
+		t.Error(`o.Get("bar") == false`)
 	}
 	if o.Get("baz") {
-		t.Error("o.Get(\"baz\") == true")
+		t.Error(`o.Get("baz") == true`)
 	}
 	o.Delete("foo")
 	if o.String() != "[bar bar]" {

opts/port.go

@@ -46,42 +46,50 @@ func (p *PortOpt) Set(value string) error {
 			// TODO(thaJeztah): these options should not be case-insensitive.
 			key, val, ok := strings.Cut(strings.ToLower(field), "=")
 			if !ok || key == "" {
-				return fmt.Errorf("invalid field %s", field)
+				return fmt.Errorf("invalid field: %s", field)
 			}
 			switch key {
 			case portOptProtocol:
 				if val != string(swarm.PortConfigProtocolTCP) && val != string(swarm.PortConfigProtocolUDP) && val != string(swarm.PortConfigProtocolSCTP) {
-					return fmt.Errorf("invalid protocol value %s", val)
+					return fmt.Errorf("invalid protocol value '%s'", val)
 				}
 
 				pConfig.Protocol = swarm.PortConfigProtocol(val)
 			case portOptMode:
 				if val != string(swarm.PortConfigPublishModeIngress) && val != string(swarm.PortConfigPublishModeHost) {
-					return fmt.Errorf("invalid publish mode value %s", val)
+					return fmt.Errorf("invalid publish mode value (%s): must be either '%s' or '%s'", val, swarm.PortConfigPublishModeIngress, swarm.PortConfigPublishModeHost)
 				}
 
 				pConfig.PublishMode = swarm.PortConfigPublishMode(val)
 			case portOptTargetPort:
 				tPort, err := strconv.ParseUint(val, 10, 16)
 				if err != nil {
-					return err
+					var numErr *strconv.NumError
+					if errors.As(err, &numErr) {
+						err = numErr.Err
+					}
+					return fmt.Errorf("invalid target port (%s): value must be an integer: %w", val, err)
 				}
 
 				pConfig.TargetPort = uint32(tPort)
 			case portOptPublishedPort:
 				pPort, err := strconv.ParseUint(val, 10, 16)
 				if err != nil {
-					return err
+					var numErr *strconv.NumError
+					if errors.As(err, &numErr) {
+						err = numErr.Err
+					}
+					return fmt.Errorf("invalid published port (%s): value must be an integer: %w", val, err)
 				}
 
 				pConfig.PublishedPort = uint32(pPort)
 			default:
-				return fmt.Errorf("invalid field key %s", key)
+				return fmt.Errorf("invalid field key: %s", key)
 			}
 		}
 
 		if pConfig.TargetPort == 0 {
-			return fmt.Errorf("missing mandatory field %q", portOptTargetPort)
+			return fmt.Errorf("missing mandatory field '%s'", portOptTargetPort)
 		}
 
 		if pConfig.PublishMode == "" {

opts/port_test.go

@@ -243,44 +243,46 @@ func TestPortOptInvalidComplexSyntax(t *testing.T) {
 	}{
 		{
 			value:         "invalid,target=80",
-			expectedError: "invalid field",
+			expectedError: "invalid field: invalid",
 		},
 		{
 			value:         "invalid=field",
-			expectedError: "invalid field",
+			expectedError: "invalid field key: invalid",
 		},
 		{
 			value:         "protocol=invalid",
-			expectedError: "invalid protocol value",
+			expectedError: "invalid protocol value 'invalid'",
 		},
 		{
 			value:         "target=invalid",
-			expectedError: "invalid syntax",
+			expectedError: "invalid target port (invalid): value must be an integer: invalid syntax",
 		},
 		{
 			value:         "published=invalid",
-			expectedError: "invalid syntax",
+			expectedError: "invalid published port (invalid): value must be an integer: invalid syntax",
 		},
 		{
 			value:         "mode=invalid",
-			expectedError: "invalid publish mode value",
+			expectedError: "invalid publish mode value (invalid): must be either 'ingress' or 'host'",
 		},
 		{
 			value:         "published=8080,protocol=tcp,mode=ingress",
-			expectedError: "missing mandatory field",
+			expectedError: "missing mandatory field 'target'",
 		},
 		{
 			value:         `target=80,protocol="tcp,mode=ingress"`,
-			expectedError: "non-quoted-field",
+			expectedError: `parse error on line 1, column 20: bare " in non-quoted-field`,
 		},
 		{
 			value:         `target=80,"protocol=tcp,mode=ingress"`,
-			expectedError: "invalid protocol value",
+			expectedError: "invalid protocol value 'tcp,mode=ingress'",
 		},
 	}
 	for _, tc := range testCases {
-		var port PortOpt
-		assert.ErrorContains(t, port.Set(tc.value), tc.expectedError)
+		t.Run(tc.value, func(t *testing.T) {
+			var port PortOpt
+			assert.Error(t, port.Set(tc.value), tc.expectedError)
+		})
 	}
 }