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.

Sign up for GitHub

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

ref(docker.go): container config fields on Driver no longer pointers #206

Merged
merged 4 commits into from
May 2, 2020
Merged

ref(docker.go): container config fields on Driver no longer pointers #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af934e4
ref(docker.go): export ApplyConfigurationOptions
vdice Apr 22, 2020
c46b74f
ref(docker.go): container config fields on Driver no longer pointers
vdice Apr 22, 2020
9b6875f
keep deep copy logic for container config accessors
vdice Apr 24, 2020
5395191
add config modification test
vdice Apr 24, 2020
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
33 changes: 17 additions & 16 deletions driver/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/docker/docker/pkg/jsonmessage"
"github.com/docker/docker/pkg/stdcopy"
"github.com/docker/docker/registry"
copystructure "github.com/mitchellh/copystructure"
"github.com/mitchellh/copystructure"
)

// Driver is capable of running Docker invocation images using Docker itself.
Expand All @@ -33,8 +33,8 @@ type Driver struct {
dockerConfigurationOptions []ConfigurationOption
containerOut io.Writer
containerErr io.Writer
containerHostCfg *container.HostConfig
containerCfg *container.Config
containerHostCfg container.HostConfig
containerCfg container.Config
}

// Run executes the Docker driver
Expand All @@ -55,33 +55,33 @@ func (d *Driver) AddConfigurationOptions(opts ...ConfigurationOption) {
// GetContainerConfig returns a copy of the container configuration
// used by the driver during container exec
func (d *Driver) GetContainerConfig() (container.Config, error) {
cpy, err := copystructure.Copy(*d.containerCfg)
cpy, err := copystructure.Copy(d.containerCfg)
if err != nil {
return container.Config{}, err
}

containerCfg, ok := cpy.(container.Config)
cfg, ok := cpy.(container.Config)
if !ok {
return container.Config{}, errors.New("unable to process container config")
}

return containerCfg, nil
return cfg, nil
}

// GetContainerHostConfig returns a copy of the container host configuration
// used by the driver during container exec
func (d *Driver) GetContainerHostConfig() (container.HostConfig, error) {
cpy, err := copystructure.Copy(*d.containerHostCfg)
cpy, err := copystructure.Copy(d.containerHostCfg)
if err != nil {
return container.HostConfig{}, err
}

hostCfg, ok := cpy.(container.HostConfig)
cfg, ok := cpy.(container.HostConfig)
if !ok {
return container.HostConfig{}, errors.New("unable to process container host config")
}

return hostCfg, nil
return cfg, nil
}

// Config returns the Docker driver configuration options
Expand Down Expand Up @@ -191,27 +191,27 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) {
env = append(env, fmt.Sprintf("%s=%v", k, v))
}

d.containerCfg = &container.Config{
d.containerCfg = container.Config{
Image: op.Image.Image,
Env: env,
Entrypoint: strslice.StrSlice{"/cnab/app/run"},
AttachStderr: true,
AttachStdout: true,
}

d.containerHostCfg = &container.HostConfig{}
if err := d.applyConfigurationOptions(); err != nil {
d.containerHostCfg = container.HostConfig{}
if err := d.ApplyConfigurationOptions(); err != nil {
return driver.OperationResult{}, err
}

resp, err := cli.Client().ContainerCreate(ctx, d.containerCfg, d.containerHostCfg, nil, "")
resp, err := cli.Client().ContainerCreate(ctx, &d.containerCfg, &d.containerHostCfg, nil, "")
switch {
case client.IsErrNotFound(err):
fmt.Fprintf(cli.Err(), "Unable to find image '%s' locally\n", op.Image.Image)
if err := pullImage(ctx, cli, op.Image.Image); err != nil {
return driver.OperationResult{}, err
}
if resp, err = cli.Client().ContainerCreate(ctx, d.containerCfg, d.containerHostCfg, nil, ""); err != nil {
if resp, err = cli.Client().ContainerCreate(ctx, &d.containerCfg, &d.containerHostCfg, nil, ""); err != nil {
return driver.OperationResult{}, fmt.Errorf("cannot create container: %v", err)
}
case err != nil:
Expand Down Expand Up @@ -293,9 +293,10 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) {
return opResult, err
}

func (d *Driver) applyConfigurationOptions() error {
// ApplyConfigurationOptions applies the configuration options set on the driver
func (d *Driver) ApplyConfigurationOptions() error {
silvin-lubecki marked this conversation as resolved.
Show resolved Hide resolved
for _, opt := range d.dockerConfigurationOptions {
if err := opt(d.containerCfg, d.containerHostCfg); err != nil {
if err := opt(&d.containerCfg, &d.containerHostCfg); err != nil {
return err
}
}
Expand Down
45 changes: 35 additions & 10 deletions driver/docker/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,19 @@ import (

"github.com/cnabio/cnab-go/driver"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/strslice"
"github.com/stretchr/testify/assert"
)

func TestDriver_GetConfigurationOptions(t *testing.T) {
d := &Driver{}
is := assert.New(t)
is.NotNil(d)
is.True(d.Handles(driver.ImageTypeDocker))

t.Run("no configuration options", func(t *testing.T) {
d.containerCfg = &container.Config{}
d.containerHostCfg = &container.HostConfig{}
t.Run("empty configuration options", func(t *testing.T) {
d := &Driver{}
is.NotNil(d)
is.True(d.Handles(driver.ImageTypeDocker))

err := d.applyConfigurationOptions()
err := d.ApplyConfigurationOptions()
is.NoError(err)

cfg, err := d.GetContainerConfig()
Expand All @@ -31,15 +30,15 @@ func TestDriver_GetConfigurationOptions(t *testing.T) {
})

t.Run("configuration options", func(t *testing.T) {
d.containerCfg = &container.Config{}
d.containerHostCfg = &container.HostConfig{}
d := &Driver{}
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 this is much cleaner to work with


d.AddConfigurationOptions(func(cfg *container.Config, hostCfg *container.HostConfig) error {
cfg.User = "cnabby"
hostCfg.Privileged = true
return nil
})

err := d.applyConfigurationOptions()
err := d.ApplyConfigurationOptions()
is.NoError(err)

expectedContainerCfg := container.Config{
Expand All @@ -57,4 +56,30 @@ func TestDriver_GetConfigurationOptions(t *testing.T) {
is.NoError(err)
is.Equal(expectedHostCfg, hostCfg)
})

t.Run("configuration options - no unintentional modification", func(t *testing.T) {
d := &Driver{}

d.AddConfigurationOptions(func(cfg *container.Config, hostCfg *container.HostConfig) error {
hostCfg.CapAdd = strslice.StrSlice{"SUPER_POWERS"}
return nil
})

err := d.ApplyConfigurationOptions()
is.NoError(err)

expectedHostCfg := container.HostConfig{
CapAdd: strslice.StrSlice{"SUPER_POWERS"},
}

hostCfg, err := d.GetContainerHostConfig()
is.NoError(err)
is.Equal(expectedHostCfg, hostCfg)

hostCfg.CapAdd[0] = "NORMAL_POWERS"

hostCfg, err = d.GetContainerHostConfig()
is.NoError(err)
is.Equal(expectedHostCfg, hostCfg)
})
}