p4u1/docker-cli
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #5907 from thaJeztah/opts_cleanup

Browse Source 
opts: remove uses pkg/errors, and move swarm-specific opts to a separate package
This commit is contained in:
Sebastiaan van Stijn
2025-03-10 18:03:49 +01:00
committed by GitHub
parent 29c1ababd7 d0d91bb0cd
commit 64b56179b5
20 changed files with 115 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
cli/command/service/create_test.go
View File

@ -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,
}

7
cli/command/service/opts.go
View File

@ -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
}

11
cli/command/service/update.go
View File

@ -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 {

3
cli/compose/loader/loader.go
View File

@ -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
}

5
opts/duration.go
View File

@ -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
}

3
opts/env.go
View File

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

10
opts/gpus.go
View File

@ -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:

18
opts/gpus_test.go
View File

@ -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{

3
opts/mount.go
View File

@ -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

6
opts/network.go
View File

@ -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)

4
opts/network_test.go
View File

@ -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) {

4
opts/opts.go
View File

@ -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 (

18
opts/opts_deprecated.go Normal file
View File

@ -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

4
opts/opts_test.go
View File

@ -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]" {

12
opts/config.go → opts/swarmopts/config.go
View File

@ -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
}

2
opts/config_test.go → opts/swarmopts/config_test.go
View File

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

24
opts/port.go → opts/swarmopts/port.go
View File

@ -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 == "" {

26
opts/port_test.go → opts/swarmopts/port_test.go
View File

@ -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)
})
}
}

12
opts/secret.go → opts/swarmopts/secret.go
View File

@ -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
}

2
opts/secret_test.go → opts/swarmopts/secret_test.go
View File

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