opts: remove uses pkg/errors, and move swarm-specific opts to a separate package

cli/command/service/create_test.go

@@ -4,12 +4,11 @@ import (
 	"context"
 	"testing"
 
+	"github.com/docker/cli/opts/swarmopts"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/swarm"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
-
-	cliopts "github.com/docker/cli/opts"
 )
 
 // fakeConfigAPIClientList is used to let us pass a closure as a
@@ -43,8 +42,8 @@ func (fakeConfigAPIClientList) ConfigUpdate(_ context.Context, _ string, _ swarm
 func TestSetConfigsWithCredSpecAndConfigs(t *testing.T) {
 	// we can't directly access the internal fields of the ConfigOpt struct, so
 	// we need to let it do the parsing
-	configOpt := &cliopts.ConfigOpt{}
-	configOpt.Set("bar")
+	configOpt := &swarmopts.ConfigOpt{}
+	assert.Check(t, configOpt.Set("bar"))
 	opts := &serviceOptions{
 		credentialSpec: credentialSpecOpt{
 			value: &swarm.CredentialSpec{
@@ -187,8 +186,8 @@ func TestSetConfigsOnlyCredSpec(t *testing.T) {
 // TestSetConfigsOnlyConfigs verifies setConfigs when only configs (and not a
 // CredentialSpec) is needed.
 func TestSetConfigsOnlyConfigs(t *testing.T) {
-	configOpt := &cliopts.ConfigOpt{}
-	configOpt.Set("bar")
+	configOpt := &swarmopts.ConfigOpt{}
+	assert.Check(t, configOpt.Set("bar"))
 	opts := &serviceOptions{
 		configs: *configOpt,
 	}

cli/command/service/opts.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/opts"
+	"github.com/docker/cli/opts/swarmopts"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/network"
 	"github.com/docker/docker/api/types/swarm"
@@ -395,7 +396,7 @@ func convertNetworks(networks opts.NetworkOpt) []swarm.NetworkAttachmentConfig {
 
 type endpointOptions struct {
 	mode         string
-	publishPorts opts.PortOpt
+	publishPorts swarmopts.PortOpt
 }
 
 func (e *endpointOptions) ToEndpointSpec() *swarm.EndpointSpec {
@@ -553,8 +554,8 @@ type serviceOptions struct {
 	logDriver logDriverOptions
 
 	healthcheck healthCheckOptions
-	secrets     opts.SecretOpt
-	configs     opts.ConfigOpt
+	secrets     swarmopts.SecretOpt
+	configs     swarmopts.ConfigOpt
 
 	isolation string
 }

cli/command/service/update.go

@@ -11,6 +11,7 @@ import (
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/completion"
 	"github.com/docker/cli/opts"
+	"github.com/docker/cli/opts/swarmopts"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
 	mounttypes "github.com/docker/docker/api/types/mount"
@@ -55,7 +56,7 @@ func newUpdateCommand(dockerCLI command.Cli) *cobra.Command {
 	flags.Var(newListOptsVar(), flagContainerLabelRemove, "Remove a container label by its key")
 	flags.Var(newListOptsVar(), flagMountRemove, "Remove a mount by its target path")
 	// flags.Var(newListOptsVar().WithValidator(validatePublishRemove), flagPublishRemove, "Remove a published port by its target port")
-	flags.Var(&opts.PortOpt{}, flagPublishRemove, "Remove a published port by its target port")
+	flags.Var(&swarmopts.PortOpt{}, flagPublishRemove, "Remove a published port by its target port")
 	flags.Var(newListOptsVar(), flagConstraintRemove, "Remove a constraint")
 	flags.Var(newListOptsVar(), flagDNSRemove, "Remove a custom DNS server")
 	flags.SetAnnotation(flagDNSRemove, "version", []string{"1.25"})
@@ -804,7 +805,7 @@ func getUpdatedSecrets(ctx context.Context, apiClient client.SecretAPIClient, fl
 	}
 
 	if flags.Changed(flagSecretAdd) {
-		values := flags.Lookup(flagSecretAdd).Value.(*opts.SecretOpt).Value()
+		values := flags.Lookup(flagSecretAdd).Value.(*swarmopts.SecretOpt).Value()
 
 		addSecrets, err := ParseSecrets(ctx, apiClient, values)
 		if err != nil {
@@ -852,7 +853,7 @@ func getUpdatedConfigs(ctx context.Context, apiClient client.ConfigAPIClient, fl
 	resolveConfigs := []*swarm.ConfigReference{}
 
 	if flags.Changed(flagConfigAdd) {
-		resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()...)
+		resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*swarmopts.ConfigOpt).Value()...)
 	}
 
 	// if credSpecConfigNameis non-empty at this point, it means its a new
@@ -1091,7 +1092,7 @@ func updatePorts(flags *pflag.FlagSet, portConfig *[]swarm.PortConfig) error {
 	newPorts := []swarm.PortConfig{}
 
 	// Clean current ports
-	toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.PortOpt).Value()
+	toRemove := flags.Lookup(flagPublishRemove).Value.(*swarmopts.PortOpt).Value()
 portLoop:
 	for _, port := range portSet {
 		for _, pConfig := range toRemove {
@@ -1107,7 +1108,7 @@ portLoop:
 
 	// Check to see if there are any conflict in flags.
 	if flags.Changed(flagPublishAdd) {
-		ports := flags.Lookup(flagPublishAdd).Value.(*opts.PortOpt).Value()
+		ports := flags.Lookup(flagPublishAdd).Value.(*swarmopts.PortOpt).Value()
 
 		for _, port := range ports {
 			if _, ok := portSet[portConfigToString(&port)]; ok {

cli/compose/loader/loader.go

@@ -18,6 +18,7 @@ import (
 	"github.com/docker/cli/cli/compose/template"
 	"github.com/docker/cli/cli/compose/types"
 	"github.com/docker/cli/opts"
+	"github.com/docker/cli/opts/swarmopts"
 	"github.com/docker/docker/api/types/versions"
 	"github.com/docker/go-connections/nat"
 	units "github.com/docker/go-units"
@@ -925,7 +926,7 @@ func toServicePortConfigs(value string) ([]any, error) {
 
 	for _, key := range keys {
 		// Reuse ConvertPortToPortConfig so that it is consistent
-		portConfig, err := opts.ConvertPortToPortConfig(nat.Port(key), portBindings)
+		portConfig, err := swarmopts.ConvertPortToPortConfig(nat.Port(key), portBindings)
 		if err != nil {
 			return nil, err
 		}

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_deprecated.go

@@ -0,0 +1,18 @@
+package opts
+
+import "github.com/docker/cli/opts/swarmopts"
+
+// PortOpt represents a port config in swarm mode.
+//
+// Deprecated: use [swarmopts.PortOpt]
+type PortOpt = swarmopts.PortOpt
+
+// ConfigOpt is a Value type for parsing configs.
+//
+// Deprecated: use [swarmopts.ConfigOpt]
+type ConfigOpt = swarmopts.ConfigOpt
+
+// SecretOpt is a Value type for parsing secrets
+//
+// Deprecated: use [swarmopts.SecretOpt]
+type SecretOpt = swarmopts.SecretOpt

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/config.go → opts/swarmopts/config.go

@@ -1,4 +1,4 @@
-package opts
+package swarmopts
 
 import (
 	"encoding/csv"
@@ -8,12 +8,12 @@ import (
 	"strconv"
 	"strings"
 
-	swarmtypes "github.com/docker/docker/api/types/swarm"
+	"github.com/docker/docker/api/types/swarm"
 )
 
 // ConfigOpt is a Value type for parsing configs
 type ConfigOpt struct {
-	values []*swarmtypes.ConfigReference
+	values []*swarm.ConfigReference
 }
 
 // Set a new config value
@@ -24,8 +24,8 @@ func (o *ConfigOpt) Set(value string) error {
 		return err
 	}
 
-	options := &swarmtypes.ConfigReference{
-		File: &swarmtypes.ConfigReferenceFileTarget{
+	options := &swarm.ConfigReference{
+		File: &swarm.ConfigReferenceFileTarget{
 			UID:  "0",
 			GID:  "0",
 			Mode: 0o444,
@@ -95,6 +95,6 @@ func (o *ConfigOpt) String() string {
 }
 
 // Value returns the config requests
-func (o *ConfigOpt) Value() []*swarmtypes.ConfigReference {
+func (o *ConfigOpt) Value() []*swarm.ConfigReference {
 	return o.values
 }

opts/config_test.go → opts/swarmopts/config_test.go

@@ -1,4 +1,4 @@
-package opts
+package swarmopts
 
 import (
 	"os"