From 9e08776681c5506eca64d404ae87d27bde18d3e5 Mon Sep 17 00:00:00 2001 From: Michael Tews Date: Wed, 23 Jul 2025 08:31:48 +0200 Subject: [PATCH] refactor(cli/compose/loader): extract ParseVolume() to its own package Moves ParseVolume() to a new internal package to remove the dependency on cli/compose/loader in cli/command/container/opts.go refactor to keep types isolated - rename the package to "volumespec" to reuse the name of the package as part of the name (parsevolume.ParseVolume() -> volumespec.Parse()) - move the related compose types to the internal package as well, and rename them to be more generic (not associated with "compose"); - ServiceVolumeConfig -> VolumeConfig - ServiceVolumeBind -> BindOpts - ServiceVolumeVolume -> VolumeOpts - ServiceVolumeImage -> ImageOpts - ServiceVolumeTmpfs -> TmpFsOpts - ServiceVolumeCluster -> ClusterOpts - alias the internal types inside cli/compose/types to keep backward compatibility (for any external consumers); even though the implementation is internal, Go allows aliasing types to use them externally. Signed-off-by: Michael Tews Signed-off-by: Sebastiaan van Stijn (cherry picked from commit ef7fd8bb678d59b29e8f69b284e7e292efdff893) Signed-off-by: Sebastiaan van Stijn --- cli/command/container/opts.go | 4 +- cli/compose/loader/loader.go | 10 ++- cli/compose/types/types.go | 34 +++------- internal/volumespec/types.go | 40 +++++++++++ .../volumespec/volumespec.go | 17 +++-- .../volumespec/volumespec_test.go | 67 +++++++++---------- 6 files changed, 100 insertions(+), 72 deletions(-) create mode 100644 internal/volumespec/types.go rename cli/compose/loader/volume.go => internal/volumespec/volumespec.go (82%) rename cli/compose/loader/volume_test.go => internal/volumespec/volumespec_test.go (77%) diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index 16b9176db9..5ff39c4ee0 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -12,8 +12,8 @@ import ( "strings" "time" - "github.com/docker/cli/cli/compose/loader" "github.com/docker/cli/internal/lazyregexp" + "github.com/docker/cli/internal/volumespec" "github.com/docker/cli/opts" "github.com/docker/docker/api/types/container" mounttypes "github.com/docker/docker/api/types/mount" @@ -374,7 +374,7 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con volumes := copts.volumes.GetMap() // add any bind targets to the list of container volumes for bind := range copts.volumes.GetMap() { - parsed, err := loader.ParseVolume(bind) + parsed, err := volumespec.Parse(bind) if err != nil { return nil, err } diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index b2673394b5..839a258f0b 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -17,6 +17,7 @@ import ( "github.com/docker/cli/cli/compose/schema" "github.com/docker/cli/cli/compose/template" "github.com/docker/cli/cli/compose/types" + "github.com/docker/cli/internal/volumespec" "github.com/docker/cli/opts" "github.com/docker/cli/opts/swarmopts" "github.com/docker/docker/api/types/versions" @@ -41,6 +42,13 @@ type Options struct { discardEnvFiles bool } +// ParseVolume parses a volume spec without any knowledge of the target platform. +// +// This function is unused, but kept for backward-compatibility for external users. +func ParseVolume(spec string) (types.ServiceVolumeConfig, error) { + return volumespec.Parse(spec) +} + // WithDiscardEnvFiles sets the Options to discard the `env_file` section after resolving to // the `environment` section func WithDiscardEnvFiles(options *Options) { @@ -756,7 +764,7 @@ var transformBuildConfig TransformerFunc = func(data any) (any, error) { var transformServiceVolumeConfig TransformerFunc = func(data any) (any, error) { switch value := data.(type) { case string: - return ParseVolume(value) + return volumespec.Parse(value) case map[string]any: return data, nil default: diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index 0804388a57..9d0c11b409 100644 --- a/cli/compose/types/types.go +++ b/cli/compose/types/types.go @@ -8,6 +8,8 @@ import ( "fmt" "strconv" "time" + + "github.com/docker/cli/internal/volumespec" ) // UnsupportedProperties not yet supported by this implementation of the compose file @@ -390,43 +392,23 @@ type ServicePortConfig struct { } // ServiceVolumeConfig are references to a volume used by a service -type ServiceVolumeConfig struct { - Type string `yaml:",omitempty" json:"type,omitempty"` - Source string `yaml:",omitempty" json:"source,omitempty"` - Target string `yaml:",omitempty" json:"target,omitempty"` - ReadOnly bool `mapstructure:"read_only" yaml:"read_only,omitempty" json:"read_only,omitempty"` - Consistency string `yaml:",omitempty" json:"consistency,omitempty"` - Bind *ServiceVolumeBind `yaml:",omitempty" json:"bind,omitempty"` - Volume *ServiceVolumeVolume `yaml:",omitempty" json:"volume,omitempty"` - Image *ServiceVolumeImage `yaml:",omitempty" json:"image,omitempty"` - Tmpfs *ServiceVolumeTmpfs `yaml:",omitempty" json:"tmpfs,omitempty"` - Cluster *ServiceVolumeCluster `yaml:",omitempty" json:"cluster,omitempty"` -} +type ServiceVolumeConfig = volumespec.VolumeConfig // ServiceVolumeBind are options for a service volume of type bind -type ServiceVolumeBind struct { - Propagation string `yaml:",omitempty" json:"propagation,omitempty"` -} +type ServiceVolumeBind = volumespec.BindOpts // ServiceVolumeVolume are options for a service volume of type volume -type ServiceVolumeVolume struct { - NoCopy bool `mapstructure:"nocopy" yaml:"nocopy,omitempty" json:"nocopy,omitempty"` - Subpath string `mapstructure:"subpath" yaml:"subpath,omitempty" json:"subpath,omitempty"` -} +type ServiceVolumeVolume = volumespec.VolumeOpts // ServiceVolumeImage are options for a service volume of type image -type ServiceVolumeImage struct { - Subpath string `mapstructure:"subpath" yaml:"subpath,omitempty" json:"subpath,omitempty"` -} +type ServiceVolumeImage = volumespec.ImageOpts // ServiceVolumeTmpfs are options for a service volume of type tmpfs -type ServiceVolumeTmpfs struct { - Size int64 `yaml:",omitempty" json:"size,omitempty"` -} +type ServiceVolumeTmpfs = volumespec.TmpFsOpts // ServiceVolumeCluster are options for a service volume of type cluster. // Deliberately left blank for future options, but unused now. -type ServiceVolumeCluster struct{} +type ServiceVolumeCluster = volumespec.ClusterOpts // FileReferenceConfig for a reference to a swarm file object type FileReferenceConfig struct { diff --git a/internal/volumespec/types.go b/internal/volumespec/types.go new file mode 100644 index 0000000000..7eb9a5006e --- /dev/null +++ b/internal/volumespec/types.go @@ -0,0 +1,40 @@ +package volumespec + +// VolumeConfig are references to a volume used by a service +type VolumeConfig struct { + Type string `yaml:",omitempty" json:"type,omitempty"` + Source string `yaml:",omitempty" json:"source,omitempty"` + Target string `yaml:",omitempty" json:"target,omitempty"` + ReadOnly bool `mapstructure:"read_only" yaml:"read_only,omitempty" json:"read_only,omitempty"` + Consistency string `yaml:",omitempty" json:"consistency,omitempty"` + Bind *BindOpts `yaml:",omitempty" json:"bind,omitempty"` + Volume *VolumeOpts `yaml:",omitempty" json:"volume,omitempty"` + Image *ImageOpts `yaml:",omitempty" json:"image,omitempty"` + Tmpfs *TmpFsOpts `yaml:",omitempty" json:"tmpfs,omitempty"` + Cluster *ClusterOpts `yaml:",omitempty" json:"cluster,omitempty"` +} + +// BindOpts are options for a service volume of type bind +type BindOpts struct { + Propagation string `yaml:",omitempty" json:"propagation,omitempty"` +} + +// VolumeOpts are options for a service volume of type volume +type VolumeOpts struct { + NoCopy bool `mapstructure:"nocopy" yaml:"nocopy,omitempty" json:"nocopy,omitempty"` + Subpath string `mapstructure:"subpath" yaml:"subpath,omitempty" json:"subpath,omitempty"` +} + +// ImageOpts are options for a service volume of type image +type ImageOpts struct { + Subpath string `mapstructure:"subpath" yaml:"subpath,omitempty" json:"subpath,omitempty"` +} + +// TmpFsOpts are options for a service volume of type tmpfs +type TmpFsOpts struct { + Size int64 `yaml:",omitempty" json:"size,omitempty"` +} + +// ClusterOpts are options for a service volume of type cluster. +// Deliberately left blank for future options, but unused now. +type ClusterOpts struct{} diff --git a/cli/compose/loader/volume.go b/internal/volumespec/volumespec.go similarity index 82% rename from cli/compose/loader/volume.go rename to internal/volumespec/volumespec.go index f043f4aa57..0ae4f31c8b 100644 --- a/cli/compose/loader/volume.go +++ b/internal/volumespec/volumespec.go @@ -1,20 +1,19 @@ -package loader +package volumespec import ( "strings" "unicode" "unicode/utf8" - "github.com/docker/cli/cli/compose/types" "github.com/docker/docker/api/types/mount" "github.com/pkg/errors" ) const endOfSpec = rune(0) -// ParseVolume parses a volume spec without any knowledge of the target platform -func ParseVolume(spec string) (types.ServiceVolumeConfig, error) { - volume := types.ServiceVolumeConfig{} +// Parse parses a volume spec without any knowledge of the target platform +func Parse(spec string) (VolumeConfig, error) { + volume := VolumeConfig{} switch len(spec) { case 0: @@ -49,7 +48,7 @@ func isWindowsDrive(buffer []rune, char rune) bool { return char == ':' && len(buffer) == 1 && unicode.IsLetter(buffer[0]) } -func populateFieldFromBuffer(char rune, buffer []rune, volume *types.ServiceVolumeConfig) error { +func populateFieldFromBuffer(char rune, buffer []rune, volume *VolumeConfig) error { strBuffer := string(buffer) switch { case len(buffer) == 0: @@ -74,10 +73,10 @@ func populateFieldFromBuffer(char rune, buffer []rune, volume *types.ServiceVolu case "rw": volume.ReadOnly = false case "nocopy": - volume.Volume = &types.ServiceVolumeVolume{NoCopy: true} + volume.Volume = &VolumeOpts{NoCopy: true} default: if isBindOption(option) { - volume.Bind = &types.ServiceVolumeBind{Propagation: option} + volume.Bind = &BindOpts{Propagation: option} } // ignore unknown options } @@ -94,7 +93,7 @@ func isBindOption(option string) bool { return false } -func populateType(volume *types.ServiceVolumeConfig) { +func populateType(volume *VolumeConfig) { switch { // Anonymous volume case volume.Source == "": diff --git a/cli/compose/loader/volume_test.go b/internal/volumespec/volumespec_test.go similarity index 77% rename from cli/compose/loader/volume_test.go rename to internal/volumespec/volumespec_test.go index 54dc200fe8..b4d356fffa 100644 --- a/cli/compose/loader/volume_test.go +++ b/internal/volumespec/volumespec_test.go @@ -1,18 +1,17 @@ -package loader +package volumespec import ( "fmt" "testing" - "github.com/docker/cli/cli/compose/types" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) func TestParseVolumeAnonymousVolume(t *testing.T) { for _, path := range []string{"/path", "/path/foo"} { - volume, err := ParseVolume(path) - expected := types.ServiceVolumeConfig{Type: "volume", Target: path} + volume, err := Parse(path) + expected := VolumeConfig{Type: "volume", Target: path} assert.NilError(t, err) assert.Check(t, is.DeepEqual(expected, volume)) } @@ -20,22 +19,22 @@ func TestParseVolumeAnonymousVolume(t *testing.T) { func TestParseVolumeAnonymousVolumeWindows(t *testing.T) { for _, path := range []string{"C:\\path", "Z:\\path\\foo"} { - volume, err := ParseVolume(path) - expected := types.ServiceVolumeConfig{Type: "volume", Target: path} + volume, err := Parse(path) + expected := VolumeConfig{Type: "volume", Target: path} assert.NilError(t, err) assert.Check(t, is.DeepEqual(expected, volume)) } } func TestParseVolumeTooManyColons(t *testing.T) { - _, err := ParseVolume("/foo:/foo:ro:foo") + _, err := Parse("/foo:/foo:ro:foo") assert.Error(t, err, "invalid spec: /foo:/foo:ro:foo: too many colons") } func TestParseVolumeShortVolumes(t *testing.T) { for _, path := range []string{".", "/a"} { - volume, err := ParseVolume(path) - expected := types.ServiceVolumeConfig{Type: "volume", Target: path} + volume, err := Parse(path) + expected := VolumeConfig{Type: "volume", Target: path} assert.NilError(t, err) assert.Check(t, is.DeepEqual(expected, volume)) } @@ -43,15 +42,15 @@ func TestParseVolumeShortVolumes(t *testing.T) { func TestParseVolumeMissingSource(t *testing.T) { for _, spec := range []string{":foo", "/foo::ro"} { - _, err := ParseVolume(spec) + _, err := Parse(spec) assert.ErrorContains(t, err, "empty section between colons") } } func TestParseVolumeBindMount(t *testing.T) { for _, path := range []string{"./foo", "~/thing", "../other", "/foo", "/home/user"} { - volume, err := ParseVolume(path + ":/target") - expected := types.ServiceVolumeConfig{ + volume, err := Parse(path + ":/target") + expected := VolumeConfig{ Type: "bind", Source: path, Target: "/target", @@ -68,8 +67,8 @@ func TestParseVolumeRelativeBindMountWindows(t *testing.T) { "../other", "D:\\path", "/home/user", } { - volume, err := ParseVolume(path + ":d:\\target") - expected := types.ServiceVolumeConfig{ + volume, err := Parse(path + ":d:\\target") + expected := VolumeConfig{ Type: "bind", Source: path, Target: "d:\\target", @@ -80,42 +79,42 @@ func TestParseVolumeRelativeBindMountWindows(t *testing.T) { } func TestParseVolumeWithBindOptions(t *testing.T) { - volume, err := ParseVolume("/source:/target:slave") - expected := types.ServiceVolumeConfig{ + volume, err := Parse("/source:/target:slave") + expected := VolumeConfig{ Type: "bind", Source: "/source", Target: "/target", - Bind: &types.ServiceVolumeBind{Propagation: "slave"}, + Bind: &BindOpts{Propagation: "slave"}, } assert.NilError(t, err) assert.Check(t, is.DeepEqual(expected, volume)) } func TestParseVolumeWithBindOptionsWindows(t *testing.T) { - volume, err := ParseVolume("C:\\source\\foo:D:\\target:ro,rprivate") - expected := types.ServiceVolumeConfig{ + volume, err := Parse("C:\\source\\foo:D:\\target:ro,rprivate") + expected := VolumeConfig{ Type: "bind", Source: "C:\\source\\foo", Target: "D:\\target", ReadOnly: true, - Bind: &types.ServiceVolumeBind{Propagation: "rprivate"}, + Bind: &BindOpts{Propagation: "rprivate"}, } assert.NilError(t, err) assert.Check(t, is.DeepEqual(expected, volume)) } func TestParseVolumeWithInvalidVolumeOptions(t *testing.T) { - _, err := ParseVolume("name:/target:bogus") + _, err := Parse("name:/target:bogus") assert.NilError(t, err) } func TestParseVolumeWithVolumeOptions(t *testing.T) { - volume, err := ParseVolume("name:/target:nocopy") - expected := types.ServiceVolumeConfig{ + volume, err := Parse("name:/target:nocopy") + expected := VolumeConfig{ Type: "volume", Source: "name", Target: "/target", - Volume: &types.ServiceVolumeVolume{NoCopy: true}, + Volume: &VolumeOpts{NoCopy: true}, } assert.NilError(t, err) assert.Check(t, is.DeepEqual(expected, volume)) @@ -123,8 +122,8 @@ func TestParseVolumeWithVolumeOptions(t *testing.T) { func TestParseVolumeWithReadOnly(t *testing.T) { for _, path := range []string{"./foo", "/home/user"} { - volume, err := ParseVolume(path + ":/target:ro") - expected := types.ServiceVolumeConfig{ + volume, err := Parse(path + ":/target:ro") + expected := VolumeConfig{ Type: "bind", Source: path, Target: "/target", @@ -137,8 +136,8 @@ func TestParseVolumeWithReadOnly(t *testing.T) { func TestParseVolumeWithRW(t *testing.T) { for _, path := range []string{"./foo", "/home/user"} { - volume, err := ParseVolume(path + ":/target:rw") - expected := types.ServiceVolumeConfig{ + volume, err := Parse(path + ":/target:rw") + expected := VolumeConfig{ Type: "bind", Source: path, Target: "/target", @@ -150,9 +149,9 @@ func TestParseVolumeWithRW(t *testing.T) { } func TestParseVolumeWindowsNamedPipe(t *testing.T) { - volume, err := ParseVolume(`\\.\pipe\docker_engine:\\.\pipe\inside`) + volume, err := Parse(`\\.\pipe\docker_engine:\\.\pipe\inside`) assert.NilError(t, err) - expected := types.ServiceVolumeConfig{ + expected := VolumeConfig{ Type: "bind", Source: `\\.\pipe\docker_engine`, Target: `\\.\pipe\inside`, @@ -206,7 +205,7 @@ func TestParseVolumeSplitCases(t *testing.T) { // Cover directories with one-character name {`/tmp/x/y:/foo/x/y`, -1, []string{`/tmp/x/y`, `/foo/x/y`}}, } { - parsed, _ := ParseVolume(x.input) + parsed, _ := Parse(x.input) expected := len(x.expected) > 1 msg := fmt.Sprintf("Case %d: %s", casenumber, x.input) @@ -215,16 +214,16 @@ func TestParseVolumeSplitCases(t *testing.T) { } func TestParseVolumeInvalidEmptySpec(t *testing.T) { - _, err := ParseVolume("") + _, err := Parse("") assert.ErrorContains(t, err, "invalid empty volume spec") } func TestParseVolumeInvalidSections(t *testing.T) { - _, err := ParseVolume("/foo::rw") + _, err := Parse("/foo::rw") assert.ErrorContains(t, err, "invalid spec") } func TestParseVolumeWithEmptySource(t *testing.T) { - _, err := ParseVolume(":/vol") + _, err := Parse(":/vol") assert.ErrorContains(t, err, "empty section between colons") }