Skip to content

Commit 64b5617

Browse files
thaJeztahthaJeztah
authored
Merge pull request #5907 from thaJeztah/opts_cleanup
opts: remove uses pkg/errors, and move swarm-specific opts to a separate package
2 parents 29c1aba + d0d91bb commit 64b5617

File tree

20 files changed

+115
-70
lines changed

20 files changed

+115
-70
lines changed

‎cli/command/service/create_test.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,11 @@ import (
44
"context"
55
"testing"
66

7+
"github.com/docker/cli/opts/swarmopts"
78
"github.com/docker/docker/api/types"
89
"github.com/docker/docker/api/types/swarm"
910
"gotest.tools/v3/assert"
1011
is "gotest.tools/v3/assert/cmp"
11-
12-
cliopts "github.com/docker/cli/opts"
1312
)
1413

1514
// fakeConfigAPIClientList is used to let us pass a closure as a
@@ -43,8 +42,8 @@ func (fakeConfigAPIClientList) ConfigUpdate(_ context.Context, _ string, _ swarm
4342
func TestSetConfigsWithCredSpecAndConfigs(t *testing.T) {
4443
// we can't directly access the internal fields of the ConfigOpt struct, so
4544
// we need to let it do the parsing
46-
configOpt := &cliopts.ConfigOpt{}
47-
configOpt.Set("bar")
45+
configOpt := &swarmopts.ConfigOpt{}
46+
assert.Check(t, configOpt.Set("bar"))
4847
opts := &serviceOptions{
4948
credentialSpec: credentialSpecOpt{
5049
value: &swarm.CredentialSpec{
@@ -187,8 +186,8 @@ func TestSetConfigsOnlyCredSpec(t *testing.T) {
187186
// TestSetConfigsOnlyConfigs verifies setConfigs when only configs (and not a
188187
// CredentialSpec) is needed.
189188
func TestSetConfigsOnlyConfigs(t *testing.T) {
190-
configOpt := &cliopts.ConfigOpt{}
191-
configOpt.Set("bar")
189+
configOpt := &swarmopts.ConfigOpt{}
190+
assert.Check(t, configOpt.Set("bar"))
192191
opts := &serviceOptions{
193192
configs: *configOpt,
194193
}

‎cli/command/service/opts.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313

1414
"github.com/docker/cli/cli/command"
1515
"github.com/docker/cli/opts"
16+
"github.com/docker/cli/opts/swarmopts"
1617
"github.com/docker/docker/api/types/container"
1718
"github.com/docker/docker/api/types/network"
1819
"github.com/docker/docker/api/types/swarm"
@@ -395,7 +396,7 @@ func convertNetworks(networks opts.NetworkOpt) []swarm.NetworkAttachmentConfig {
395396

396397
type endpointOptions struct {
397398
mode string
398-
publishPorts opts.PortOpt
399+
publishPorts swarmopts.PortOpt
399400
}
400401

401402
func (e *endpointOptions) ToEndpointSpec() *swarm.EndpointSpec {
@@ -553,8 +554,8 @@ type serviceOptions struct {
553554
logDriver logDriverOptions
554555

555556
healthcheck healthCheckOptions
556-
secrets opts.SecretOpt
557-
configs opts.ConfigOpt
557+
secrets swarmopts.SecretOpt
558+
configs swarmopts.ConfigOpt
558559

559560
isolation string
560561
}

‎cli/command/service/update.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/docker/cli/cli/command"
1212
"github.com/docker/cli/cli/command/completion"
1313
"github.com/docker/cli/opts"
14+
"github.com/docker/cli/opts/swarmopts"
1415
"github.com/docker/docker/api/types"
1516
"github.com/docker/docker/api/types/container"
1617
mounttypes "github.com/docker/docker/api/types/mount"
@@ -55,7 +56,7 @@ func newUpdateCommand(dockerCLI command.Cli) *cobra.Command {
5556
flags.Var(newListOptsVar(), flagContainerLabelRemove, "Remove a container label by its key")
5657
flags.Var(newListOptsVar(), flagMountRemove, "Remove a mount by its target path")
5758
// flags.Var(newListOptsVar().WithValidator(validatePublishRemove), flagPublishRemove, "Remove a published port by its target port")
58-
flags.Var(&opts.PortOpt{}, flagPublishRemove, "Remove a published port by its target port")
59+
flags.Var(&swarmopts.PortOpt{}, flagPublishRemove, "Remove a published port by its target port")
5960
flags.Var(newListOptsVar(), flagConstraintRemove, "Remove a constraint")
6061
flags.Var(newListOptsVar(), flagDNSRemove, "Remove a custom DNS server")
6162
flags.SetAnnotation(flagDNSRemove, "version", []string{"1.25"})
@@ -804,7 +805,7 @@ func getUpdatedSecrets(ctx context.Context, apiClient client.SecretAPIClient, fl
804805
}
805806

806807
if flags.Changed(flagSecretAdd) {
807-
values := flags.Lookup(flagSecretAdd).Value.(*opts.SecretOpt).Value()
808+
values := flags.Lookup(flagSecretAdd).Value.(*swarmopts.SecretOpt).Value()
808809

809810
addSecrets, err := ParseSecrets(ctx, apiClient, values)
810811
if err != nil {
@@ -852,7 +853,7 @@ func getUpdatedConfigs(ctx context.Context, apiClient client.ConfigAPIClient, fl
852853
resolveConfigs := []*swarm.ConfigReference{}
853854

854855
if flags.Changed(flagConfigAdd) {
855-
resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()...)
856+
resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*swarmopts.ConfigOpt).Value()...)
856857
}
857858

858859
// 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 {
10911092
newPorts := []swarm.PortConfig{}
10921093

10931094
// Clean current ports
1094-
toRemove := flags.Lookup(flagPublishRemove).Value.(*opts.PortOpt).Value()
1095+
toRemove := flags.Lookup(flagPublishRemove).Value.(*swarmopts.PortOpt).Value()
10951096
portLoop:
10961097
for _, port := range portSet {
10971098
for _, pConfig := range toRemove {
@@ -1107,7 +1108,7 @@ portLoop:
11071108

11081109
// Check to see if there are any conflict in flags.
11091110
if flags.Changed(flagPublishAdd) {
1110-
ports := flags.Lookup(flagPublishAdd).Value.(*opts.PortOpt).Value()
1111+
ports := flags.Lookup(flagPublishAdd).Value.(*swarmopts.PortOpt).Value()
11111112

11121113
for _, port := range ports {
11131114
if _, ok := portSet[portConfigToString(&port)]; ok {

‎cli/compose/loader/loader.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/docker/cli/cli/compose/template"
1919
"github.com/docker/cli/cli/compose/types"
2020
"github.com/docker/cli/opts"
21+
"github.com/docker/cli/opts/swarmopts"
2122
"github.com/docker/docker/api/types/versions"
2223
"github.com/docker/go-connections/nat"
2324
units "github.com/docker/go-units"
@@ -925,7 +926,7 @@ func toServicePortConfigs(value string) ([]any, error) {
925926

926927
for _, key := range keys {
927928
// Reuse ConvertPortToPortConfig so that it is consistent
928-
portConfig, err := opts.ConvertPortToPortConfig(nat.Port(key), portBindings)
929+
portConfig, err := swarmopts.ConvertPortToPortConfig(nat.Port(key), portBindings)
929930
if err != nil {
930931
return nil, err
931932
}

‎opts/duration.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
package opts
22

33
import (
4+
"errors"
45
"time"
5-
6-
"github.com/pkg/errors"
76
)
87

98
// PositiveDurationOpt is an option type for time.Duration that uses a pointer.
@@ -20,7 +19,7 @@ func (d *PositiveDurationOpt) Set(s string) error {
2019
return err
2120
}
2221
if *d.DurationOpt.value < 0 {
23-
return errors.Errorf("duration cannot be negative")
22+
return errors.New("duration cannot be negative")
2423
}
2524
return nil
2625
}

‎opts/env.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
package opts
22

33
import (
4+
"errors"
45
"os"
56
"strings"
6-
7-
"github.com/pkg/errors"
87
)
98

109
// ValidateEnv validates an environment variable and returns it.

‎opts/gpus.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@ package opts
22

33
import (
44
"encoding/csv"
5+
"errors"
56
"fmt"
67
"strconv"
78
"strings"
89

910
"github.com/docker/docker/api/types/container"
10-
"github.com/pkg/errors"
1111
)
1212

1313
// GpuOpts is a Value type for parsing mounts
@@ -21,7 +21,11 @@ func parseCount(s string) (int, error) {
2121
}
2222
i, err := strconv.Atoi(s)
2323
if err != nil {
24-
return 0, errors.Wrap(err, "count must be an integer")
24+
var numErr *strconv.NumError
25+
if errors.As(err, &numErr) {
26+
err = numErr.Err
27+
}
28+
return 0, fmt.Errorf(`invalid count (%s): value must be either "all" or an integer: %w`, s, err)
2529
}
2630
return i, nil
2731
}
@@ -72,7 +76,7 @@ func (o *GpuOpts) Set(value string) error {
7276
r := csv.NewReader(strings.NewReader(val))
7377
optFields, err := r.Read()
7478
if err != nil {
75-
return errors.Wrap(err, "failed to read gpu options")
79+
return fmt.Errorf("failed to read gpu options: %w", err)
7680
}
7781
req.Options = ConvertKVStringsToMap(optFields)
7882
default:

‎opts/gpus_test.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ func TestGpusOptAll(t *testing.T) {
1616
"count=-1",
1717
} {
1818
var gpus GpuOpts
19-
gpus.Set(testcase)
19+
assert.Check(t, gpus.Set(testcase))
2020
gpuReqs := gpus.Value()
2121
assert.Assert(t, is.Len(gpuReqs, 1))
2222
assert.Check(t, is.DeepEqual(gpuReqs[0], container.DeviceRequest{
@@ -27,15 +27,21 @@ func TestGpusOptAll(t *testing.T) {
2727
}
2828
}
2929

30+
func TestGpusOptInvalidCount(t *testing.T) {
31+
var gpus GpuOpts
32+
err := gpus.Set(`count=invalid-integer`)
33+
assert.Error(t, err, `invalid count (invalid-integer): value must be either "all" or an integer: invalid syntax`)
34+
}
35+
3036
func TestGpusOpts(t *testing.T) {
3137
for _, testcase := range []string{
32-
"driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\"",
33-
"1,driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\"",
34-
"count=1,driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\"",
35-
"driver=nvidia,\"capabilities=compute,utility\",\"options=foo=bar,baz=qux\",count=1",
38+
`driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux"`,
39+
`1,driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux"`,
40+
`count=1,driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux"`,
41+
`driver=nvidia,"capabilities=compute,utility","options=foo=bar,baz=qux",count=1`,
3642
} {
3743
var gpus GpuOpts
38-
gpus.Set(testcase)
44+
assert.Check(t, gpus.Set(testcase))
3945
gpuReqs := gpus.Value()
4046
assert.Assert(t, is.Len(gpuReqs, 1))
4147
assert.Check(t, is.DeepEqual(gpuReqs[0], container.DeviceRequest{

‎opts/mount.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,7 @@ func (m *MountOpt) Set(value string) error {
135135
// TODO: implicitly set propagation and error if the user specifies a propagation in a future refactor/UX polish pass
136136
// https://github.com/docker/cli/pull/4316#discussion_r1341974730
137137
default:
138-
return fmt.Errorf("invalid value for %s: %s (must be \"enabled\", \"disabled\", \"writable\", or \"readonly\")",
139-
key, val)
138+
return fmt.Errorf(`invalid value for %s: %s (must be "enabled", "disabled", "writable", or "readonly")`, key, val)
140139
}
141140
case "volume-subpath":
142141
volumeOptions().Subpath = val

‎opts/network.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,11 @@ func (n *NetworkOpt) Set(value string) error { //nolint:gocyclo
8989
case gwPriorityOpt:
9090
netOpt.GwPriority, err = strconv.Atoi(val)
9191
if err != nil {
92-
return fmt.Errorf("invalid gw-priority: %w", err)
92+
var numErr *strconv.NumError
93+
if errors.As(err, &numErr) {
94+
err = numErr.Err
95+
}
96+
return fmt.Errorf("invalid gw-priority (%s): %w", val, err)
9397
}
9498
default:
9599
return errors.New("invalid field key " + key)

0 commit comments

Comments
 (0)