Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: deprecate BindMount APIs #1907

Merged
merged 4 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions container.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,8 @@ func (c *ContainerRequest) validateContextOrImageIsSpecified() error {
return nil
}

// validateMounts ensures that the mounts do not have duplicate targets.
// It will check the Mounts and HostConfigModifier.Binds fields.
func (c *ContainerRequest) validateMounts() error {
Copy link
Collaborator Author

@mdelapenya mdelapenya Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think the validation functions are very little useful, as they actually do too few checks.

We probably need a follow up discussion to remove it or rethink what to check here

targets := make(map[string]bool, len(c.Mounts))

Expand All @@ -317,5 +319,29 @@ func (c *ContainerRequest) validateMounts() error {
targets[targetPath] = true
}
}

if c.HostConfigModifier == nil {
return nil
}

hostConfig := container.HostConfig{}

c.HostConfigModifier(&hostConfig)

if hostConfig.Binds != nil && len(hostConfig.Binds) > 0 {
for _, bind := range hostConfig.Binds {
parts := strings.Split(bind, ":")
if len(parts) != 2 {
return fmt.Errorf("%w: %s", ErrInvalidBindMount, bind)
}
targetPath := parts[1]
if targets[targetPath] {
return fmt.Errorf("%w: %s", ErrDuplicateMountTarget, targetPath)
} else {
targets[targetPath] = true
}
}
}

return nil
}
23 changes: 19 additions & 4 deletions container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"testing"
"time"

"github.com/docker/docker/api/types/container"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -55,16 +56,30 @@ func Test_ContainerValidation(t *testing.T) {
Name: "Can mount same source to multiple targets",
ExpectedError: nil,
ContainerRequest: ContainerRequest{
Image: "redis:latest",
Mounts: Mounts(BindMount("/data", "/srv"), BindMount("/data", "/data")),
Image: "redis:latest",
HostConfigModifier: func(hc *container.HostConfig) {
hc.Binds = []string{"/data:/srv", "/data:/data"}
},
},
},
{
Name: "Cannot mount multiple sources to same target",
ExpectedError: errors.New("duplicate mount target detected: /data"),
ContainerRequest: ContainerRequest{
Image: "redis:latest",
Mounts: Mounts(BindMount("/srv", "/data"), BindMount("/data", "/data")),
Image: "redis:latest",
HostConfigModifier: func(hc *container.HostConfig) {
hc.Binds = []string{"/data:/data", "/data:/data"}
},
},
},
{
Name: "Invalid bind mount",
ExpectedError: errors.New("invalid bind mount: /data:/data:/data"),
ContainerRequest: ContainerRequest{
Image: "redis:latest",
HostConfigModifier: func(hc *container.HostConfig) {
hc.Binds = []string{"/data:/data:/data"}
},
},
},
}
Expand Down
8 changes: 2 additions & 6 deletions docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,8 @@ import (
"github.com/testcontainers/testcontainers-go/wait"
)

var (
// Implement interfaces
_ Container = (*DockerContainer)(nil)

ErrDuplicateMountTarget = errors.New("duplicate mount target detected")
)
// Implement interfaces
var _ Container = (*DockerContainer)(nil)

const (
Bridge = "bridge" // Bridge network name (as well as driver)
Expand Down
20 changes: 8 additions & 12 deletions docker_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ func prepareLocalRegistryWithAuth(t *testing.T) {
ctx := context.Background()
wd, err := os.Getwd()
assert.NoError(t, err)
// bindMounts {
// copyDirectoryToContainer {
req := ContainerRequest{
Image: "registry:2",
ExposedPorts: []string{"5001:5000/tcp"},
Expand All @@ -253,18 +253,14 @@ func prepareLocalRegistryWithAuth(t *testing.T) {
"REGISTRY_AUTH_HTPASSWD_PATH": "/auth/htpasswd",
"REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY": "/data",
},
Mounts: ContainerMounts{
ContainerMount{
Source: GenericBindMountSource{
HostPath: fmt.Sprintf("%s/testdata/auth", wd),
},
Target: "/auth",
Files: []ContainerFile{
{
HostFilePath: fmt.Sprintf("%s/testdata/auth", wd),
ContainerFilePath: "/auth",
},
ContainerMount{
Source: GenericBindMountSource{
HostPath: fmt.Sprintf("%s/testdata/data", wd),
},
Target: "/data",
{
HostFilePath: fmt.Sprintf("%s/testdata/data", wd),
ContainerFilePath: "/data",
},
},
WaitingFor: wait.ForExposedPort(),
Expand Down
10 changes: 7 additions & 3 deletions docker_mounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ package testcontainers
import "github.com/docker/docker/api/types/mount"

var mountTypeMapping = map[MountType]mount.Type{
MountTypeBind: mount.TypeBind,
MountTypeVolume: mount.TypeVolume,
MountTypeTmpfs: mount.TypeTmpfs,
MountTypePipe: mount.TypeNamedPipe,
}

// Deprecated: use HostConfigModifier in the ContainerRequest to make containers portable across Docker environments
// BindMounter can optionally be implemented by mount sources
// to support advanced scenarios based on mount.BindOptions
type BindMounter interface {
Expand All @@ -27,6 +27,7 @@ type TmpfsMounter interface {
GetTmpfsOptions() *mount.TmpfsOptions
}

// Deprecated: use HostConfigModifier in the ContainerRequest to make containers portable across Docker environments
type DockerBindMountSource struct {
*mount.BindOptions

Expand All @@ -35,14 +36,17 @@ type DockerBindMountSource struct {
HostPath string
}

// Deprecated: use HostConfigModifier in the ContainerRequest to make containers portable across Docker environments
func (s DockerBindMountSource) Source() string {
return s.HostPath
}

// Deprecated: use HostConfigModifier in the ContainerRequest to make containers portable across Docker environments
func (DockerBindMountSource) Type() MountType {
return MountTypeBind
}

// Deprecated: use HostConfigModifier in the ContainerRequest to make containers portable across Docker environments
func (s DockerBindMountSource) GetBindOptions() *mount.BindOptions {
return s.BindOptions
}
Expand Down Expand Up @@ -99,12 +103,12 @@ func mapToDockerMounts(containerMounts ContainerMounts) []mount.Mount {
}

switch typedMounter := m.Source.(type) {
case BindMounter:
containerMount.BindOptions = typedMounter.GetBindOptions()
case VolumeMounter:
containerMount.VolumeOptions = typedMounter.GetVolumeOptions()
case TmpfsMounter:
containerMount.TmpfsOptions = typedMounter.GetTmpfsOptions()
default:
Logger.Printf("Mount type %s is not supported by Testcontainers for Go", m.Source.Type())
}

mounts = append(mounts, containerMount)
Expand Down
51 changes: 23 additions & 28 deletions docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/strslice"
"github.com/docker/docker/api/types/volume"
"github.com/docker/docker/errdefs"
"github.com/docker/go-units"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -142,8 +141,13 @@ func TestContainerWithHostNetworkOptions(t *testing.T) {
gcr := GenericContainerRequest{
ProviderType: providerType,
ContainerRequest: ContainerRequest{
Image: nginxAlpineImage,
Mounts: Mounts(BindMount(absPath, "/etc/nginx/conf.d/default.conf")),
Image: nginxAlpineImage,
Files: []ContainerFile{
{
HostFilePath: absPath,
ContainerFilePath: "/etc/nginx/conf.d/default.conf",
},
},
ExposedPorts: []string{
nginxHighPort,
},
Expand Down Expand Up @@ -258,7 +262,12 @@ func TestContainerWithHostNetwork(t *testing.T) {
ContainerRequest: ContainerRequest{
Image: nginxAlpineImage,
WaitingFor: wait.ForListeningPort(nginxHighPort),
Mounts: Mounts(BindMount(absPath, "/etc/nginx/conf.d/default.conf")),
Files: []ContainerFile{
{
HostFilePath: absPath,
ContainerFilePath: "/etc/nginx/conf.d/default.conf",
},
},
HostConfigModifier: func(hc *container.HostConfig) {
hc.NetworkMode = "host"
},
Expand Down Expand Up @@ -1187,43 +1196,29 @@ func ExampleContainer_MappedPort() {
// }
}

func TestContainerCreationWithBindAndVolume(t *testing.T) {
func TestContainerCreationWithVolumeAndFileWritingToIt(t *testing.T) {
absPath, err := filepath.Abs(filepath.Join(".", "testdata", "hello.sh"))
if err != nil {
t.Fatal(err)
}
ctx, cnl := context.WithTimeout(context.Background(), 30*time.Second)
defer cnl()
// Create a Docker client.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to instantiate a client for the volume, as Docker will handle the mounts correctly.

dockerCli, err := NewDockerClientWithOpts(context.Background())
if err != nil {
t.Fatal(err)
}

// Create the volume.
vol, err := dockerCli.VolumeCreate(ctx, volume.CreateOptions{
Driver: "local",
})
if err != nil {
t.Fatal(err)
}
volumeName := vol.Name
t.Cleanup(func() {
ctx, cnl := context.WithTimeout(context.Background(), 5*time.Second)
defer cnl()
defer dockerCli.Close()
volumeName := "volumeName"

err := dockerCli.VolumeRemove(ctx, volumeName, true)
if err != nil {
t.Fatal(err)
}
})
// Create the container that writes into the mounted volume.
bashC, err := GenericContainer(ctx, GenericContainerRequest{
ProviderType: providerType,
ContainerRequest: ContainerRequest{
Image: "docker.io/bash",
Mounts: Mounts(BindMount(absPath, "/hello.sh"), VolumeMount(volumeName, "/data")),
Image: "docker.io/bash",
Files: []ContainerFile{
{
HostFilePath: absPath,
ContainerFilePath: "/hello.sh",
},
},
Mounts: Mounts(VolumeMount(volumeName, "/data")),
Cmd: []string{"bash", "/hello.sh"},
WaitingFor: wait.ForLog("done"),
},
Expand Down
113 changes: 0 additions & 113 deletions docs/features/copy_file.md

This file was deleted.

Loading