diff --git a/cli/command/service/create_test.go b/cli/command/service/create_test.go index 169a9e2fe..9ae4f2db4 100644 --- a/cli/command/service/create_test.go +++ b/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, } diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index 983e2233b..bd5ea0435 100644 --- a/cli/command/service/opts.go +++ b/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 } diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 36f09bfa5..f69e8f4fb 100644 --- a/cli/command/service/update.go +++ b/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 { diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 6cd2d2031..e63c86e12 100644 --- a/cli/compose/loader/loader.go +++ b/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 } diff --git a/opts/duration.go b/opts/duration.go index d55c51e62..41a27a564 100644 --- a/opts/duration.go +++ b/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 } diff --git a/opts/env.go b/opts/env.go index 214d6f440..675ddda96 100644 --- a/opts/env.go +++ b/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. diff --git a/opts/gpus.go b/opts/gpus.go index 492a19083..6a56c49c4 100644 --- a/opts/gpus.go +++ b/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: diff --git a/opts/gpus_test.go b/opts/gpus_test.go index 8f620046d..ea177a197 100644 --- a/opts/gpus_test.go +++ b/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{ diff --git a/opts/mount.go b/opts/mount.go index 275a4d7f8..a4219b07f 100644 --- a/opts/mount.go +++ b/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 diff --git a/opts/network.go b/opts/network.go index c3510870e..43b3a09d4 100644 --- a/opts/network.go +++ b/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) diff --git a/opts/network_test.go b/opts/network_test.go index 71f38e98a..ffdcbf26c 100644 --- a/opts/network_test.go +++ b/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) { diff --git a/opts/opts.go b/opts/opts.go index 061fda57c..f6b337078 100644 --- a/opts/opts.go +++ b/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 ( diff --git a/opts/opts_deprecated.go b/opts/opts_deprecated.go new file mode 100644 index 000000000..2eab7a125 --- /dev/null +++ b/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 diff --git a/opts/opts_test.go b/opts/opts_test.go index a69153ca2..5169d48a6 100644 --- a/opts/opts_test.go +++ b/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]" { diff --git a/opts/config.go b/opts/swarmopts/config.go similarity index 88% rename from opts/config.go rename to opts/swarmopts/config.go index 1fc0eb356..ff137304a 100644 --- a/opts/config.go +++ b/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 } diff --git a/opts/config_test.go b/opts/swarmopts/config_test.go similarity index 99% rename from opts/config_test.go rename to opts/swarmopts/config_test.go index ab7d0c5b2..43683f412 100644 --- a/opts/config_test.go +++ b/opts/swarmopts/config_test.go @@ -1,4 +1,4 @@ -package opts +package swarmopts import ( "os" diff --git a/opts/port.go b/opts/swarmopts/port.go similarity index 84% rename from opts/port.go rename to opts/swarmopts/port.go index 0407355e6..e15c6b830 100644 --- a/opts/port.go +++ b/opts/swarmopts/port.go @@ -1,4 +1,4 @@ -package opts +package swarmopts import ( "encoding/csv" @@ -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 == "" { diff --git a/opts/port_test.go b/opts/swarmopts/port_test.go similarity index 90% rename from opts/port_test.go rename to opts/swarmopts/port_test.go index 06d19fce3..c13c6cd4d 100644 --- a/opts/port_test.go +++ b/opts/swarmopts/port_test.go @@ -1,4 +1,4 @@ -package opts +package swarmopts import ( "bytes" @@ -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) + }) } } diff --git a/opts/secret.go b/opts/swarmopts/secret.go similarity index 88% rename from opts/secret.go rename to opts/swarmopts/secret.go index bdf232de6..9f97627a5 100644 --- a/opts/secret.go +++ b/opts/swarmopts/secret.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" ) // SecretOpt is a Value type for parsing secrets type SecretOpt struct { - values []*swarmtypes.SecretReference + values []*swarm.SecretReference } // Set a new secret value @@ -24,8 +24,8 @@ func (o *SecretOpt) Set(value string) error { return err } - options := &swarmtypes.SecretReference{ - File: &swarmtypes.SecretReferenceFileTarget{ + options := &swarm.SecretReference{ + File: &swarm.SecretReferenceFileTarget{ UID: "0", GID: "0", Mode: 0o444, @@ -94,6 +94,6 @@ func (o *SecretOpt) String() string { } // Value returns the secret requests -func (o *SecretOpt) Value() []*swarmtypes.SecretReference { +func (o *SecretOpt) Value() []*swarm.SecretReference { return o.values } diff --git a/opts/secret_test.go b/opts/swarmopts/secret_test.go similarity index 99% rename from opts/secret_test.go rename to opts/swarmopts/secret_test.go index 88f4f9602..c8656f615 100644 --- a/opts/secret_test.go +++ b/opts/swarmopts/secret_test.go @@ -1,4 +1,4 @@ -package opts +package swarmopts import ( "os"