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 <michael@tews.dev>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit ef7fd8bb67)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

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
 		}

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:

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 ServiceVolumeConfig = volumespec.VolumeConfig
-	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"`
-}
 
 // ServiceVolumeBind are options for a service volume of type bind
-type ServiceVolumeBind struct {
+type ServiceVolumeBind = volumespec.BindOpts
-	Propagation string `yaml:",omitempty" json:"propagation,omitempty"`
-}
 
 // ServiceVolumeVolume are options for a service volume of type volume
-type ServiceVolumeVolume struct {
+type ServiceVolumeVolume = volumespec.VolumeOpts
-	NoCopy  bool   `mapstructure:"nocopy" yaml:"nocopy,omitempty" json:"nocopy,omitempty"`
-	Subpath string `mapstructure:"subpath" yaml:"subpath,omitempty" json:"subpath,omitempty"`
-}
 
 // ServiceVolumeImage are options for a service volume of type image
-type ServiceVolumeImage struct {
+type ServiceVolumeImage = volumespec.ImageOpts
-	Subpath string `mapstructure:"subpath" yaml:"subpath,omitempty" json:"subpath,omitempty"`
-}
 
 // ServiceVolumeTmpfs are options for a service volume of type tmpfs
-type ServiceVolumeTmpfs struct {
+type ServiceVolumeTmpfs = volumespec.TmpFsOpts
-	Size int64 `yaml:",omitempty" json:"size,omitempty"`
-}
 
 // 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 {

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

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
+// Parse parses a volume spec without any knowledge of the target platform
-func ParseVolume(spec string) (types.ServiceVolumeConfig, error) {
+func Parse(spec string) (VolumeConfig, error) {
-	volume := types.ServiceVolumeConfig{}
+	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 == "":

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)
+		volume, err := Parse(path)
-		expected := types.ServiceVolumeConfig{Type: "volume", Target: 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)
+		volume, err := Parse(path)
-		expected := types.ServiceVolumeConfig{Type: "volume", Target: 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)
+		volume, err := Parse(path)
-		expected := types.ServiceVolumeConfig{Type: "volume", Target: 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")
+		volume, err := Parse(path + ":/target")
-		expected := types.ServiceVolumeConfig{
+		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")
+		volume, err := Parse(path + ":d:\\target")
-		expected := types.ServiceVolumeConfig{
+		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")
+	volume, err := Parse("/source:/target:slave")
-	expected := types.ServiceVolumeConfig{
+	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")
+	volume, err := Parse("C:\\source\\foo:D:\\target:ro,rprivate")
-	expected := types.ServiceVolumeConfig{
+	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")
+	volume, err := Parse("name:/target:nocopy")
-	expected := types.ServiceVolumeConfig{
+	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")
+		volume, err := Parse(path + ":/target:ro")
-		expected := types.ServiceVolumeConfig{
+		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")
+		volume, err := Parse(path + ":/target:rw")
-		expected := types.ServiceVolumeConfig{
+		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")
 }