Add top-level volumes to ecsProject

SoManyHs · allisaurus · commit 16d7aa7db852 · 2018-06-04T10:55:48.000-07:00
Also changed tests for ConvertToMountPoints to better reflect actual
inputs for volumes in docker-compose, as well as to account for mutating
top-level volumes struct

TODO: set volumes on project in parseV3
diff --git a/ecs-cli/modules/cli/compose/adapter/convert_test.go b/ecs-cli/modules/cli/compose/adapter/convert_test.go
@@ -21,7 +21,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ecs"
 	"github.com/docker/libcompose/config"
-	"github.com/docker/libcompose/project"
 	"github.com/docker/libcompose/yaml"
 	"github.com/stretchr/testify/assert"
 )
@@ -205,20 +204,22 @@ func verifyPortMapping(t *testing.T, output *ecs.PortMapping, hostPort, containe
 }
 
 func TestConvertToMountPoints(t *testing.T) {
+	// Valid inputs with host and container paths set:
+	// /tmp/cache
+	// /tmp/cache2
+	// ./cache:/tmp/cache
+	// /tmp/cache:ro
+	// ./cache:/tmp/cache:ro
+	// ./cache:/tmp/cache:rw
+	// named_volume:/tmp/cache
 	onlyContainerPath := yaml.Volume{Destination: containerPath}
 	onlyContainerPath2 := yaml.Volume{Destination: containerPath2}
-	hostAndContainerPath := yaml.Volume{Source: hostPath, Destination: containerPath} // "./cache:/tmp/cache"
+	hostAndContainerPath := yaml.Volume{Source: hostPath, Destination: containerPath}
 	onlyContainerPathWithRO := yaml.Volume{Destination: containerPath, AccessMode: "ro"}
-	hostAndContainerPathWithRO := yaml.Volume{Source: hostPath, Destination: containerPath, AccessMode: "ro"} // "./cache:/tmp/cache:ro"
+	hostAndContainerPathWithRO := yaml.Volume{Source: hostPath, Destination: containerPath, AccessMode: "ro"}
 	hostAndContainerPathWithRW := yaml.Volume{Source: hostPath, Destination: containerPath, AccessMode: "rw"}
 	namedVolumeAndContainerPath := yaml.Volume{Source: namedVolume, Destination: containerPath}
 
-	volumes := &Volumes{
-		VolumeWithHost:  make(map[string]string), // map with key:=hostSourcePath value:=VolumeName
-		VolumeEmptyHost: []string{namedVolume},   // Declare one volume with an empty host
-	}
-
-	// Valid inputs with host and container paths set
 	mountPointsIn := yaml.Volumes{
 		Volumes: []*yaml.Volume{
 			&onlyContainerPath,
@@ -231,21 +232,59 @@ func TestConvertToMountPoints(t *testing.T) {
 		},
 	}
 
-	mountPointsOut, err := ConvertToMountPoints(&mountPointsIn, volumes)
-	if err != nil {
-		t.Fatalf("Expected to convert [%v] mountPoints without errors. But got [%v]", mountPointsIn, err)
+	expectedVolumeWithHost := map[string]string{hostPath: "volume-3"}
+	expectedVolumeEmptyHost := []string{namedVolume, "volume-1", "volume-2", "volume-4"}
+	expectedMountPoints := []*ecs.MountPoint{
+		{
+			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("volume-1"),
+		},
+		{
+			ContainerPath: aws.String("/tmp/cache2"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("volume-2"),
+		},
+		{
+			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("volume-3"),
+		},
+		{
+			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(true),
+			SourceVolume:  aws.String("volume-4"),
+		},
+		{
+			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(true),
+			SourceVolume:  aws.String("volume-3"),
+		},
+		{
+			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("volume-3"),
+		},
+		{
+			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("named_volume"),
+		},
 	}
-	if len(mountPointsIn.Volumes) != len(mountPointsOut) {
-		t.Errorf("Incorrect conversion. Input [%v] Output [%v]", mountPointsIn, mountPointsOut)
+
+	volumes := &Volumes{
+		VolumeWithHost:  make(map[string]string), // This field should be empty before ConvertToMountPoints is called
+		VolumeEmptyHost: []string{namedVolume},   // We expect ConvertToVolumes to already have been called, so any named volumes should have been added to VolumeEmptyHost
 	}
 
-	verifyMountPoint(t, mountPointsOut[0], volumes, "", containerPath, false, 1)  // 1 is the counter for the first volume with an empty host path
-	verifyMountPoint(t, mountPointsOut[1], volumes, "", containerPath2, false, 2) // 2 is the counter for the second volume with an empty host path
-	verifyMountPoint(t, mountPointsOut[2], volumes, hostPath, containerPath, false, 2)
-	verifyMountPoint(t, mountPointsOut[3], volumes, "", containerPath, true, 3) // 3 is the counter for the third volume with an empty host path
-	verifyMountPoint(t, mountPointsOut[4], volumes, hostPath, containerPath, true, 3)
-	verifyMountPoint(t, mountPointsOut[5], volumes, hostPath, containerPath, false, 3)
-	verifyMountPoint(t, mountPointsOut[6], volumes, namedVolume, containerPath, false, 3)
+	mountPointsOut, err := ConvertToMountPoints(&mountPointsIn, volumes)
+	assert.NoError(t, err, "Unexpected error converting MountPoints")
+
+	// Expect top-level volumes fields to be populated
+	assert.Equal(t, expectedVolumeWithHost, volumes.VolumeWithHost, "Expected volumeWithHost to match")
+	assert.Equal(t, expectedVolumeEmptyHost, volumes.VolumeEmptyHost, "Expected volumeEmptyHost to match")
+
+	assert.ElementsMatch(t, expectedMountPoints, mountPointsOut, "Expected Mount Points to match")
 
 	if mountPointsOut[0].SourceVolume == mountPointsOut[1].SourceVolume {
 		t.Errorf("Expected volume %v (onlyContainerPath) and %v (onlyContainerPath2) to be different", mountPointsOut[0].SourceVolume, mountPointsOut[1].SourceVolume)
@@ -273,10 +312,7 @@ func TestConvertToMountPointsWithInvalidAccessMode(t *testing.T) {
 	}
 
 	_, err := ConvertToMountPoints(&mountPointsIn, volumes)
-
-	if err == nil {
-		t.Errorf("Expected to get error for mountPoint[%s] but didn't.", hostAndContainerPathWithIncorrectAccess)
-	}
+	assert.Error(t, err, "Expected error converting MountPoints")
 }
 
 func TestConvertToMountPointsNullContainerVolumes(t *testing.T) {
@@ -285,33 +321,27 @@ func TestConvertToMountPointsNullContainerVolumes(t *testing.T) {
 		VolumeEmptyHost: []string{namedVolume},
 	}
 	mountPointsOut, err := ConvertToMountPoints(nil, volumes)
-	if err != nil {
-		t.Fatalf("Expected to convert nil mountPoints without errors. But got [%v]", err)
-	}
-	if len(mountPointsOut) != 0 {
-		t.Errorf("Incorrect conversion. Input nil Output [%v]", mountPointsOut)
-	}
+	assert.NoError(t, err, "Unexpected error converting MountPoints")
+	assert.Empty(t, mountPointsOut, "Expected mount points to be empty")
 }
 
-func verifyMountPoint(t *testing.T, output *ecs.MountPoint, volumes *Volumes,
-	source, containerPath string, readonly bool, EmptyHostCtr int) {
-	sourceVolume := ""
-	if containerPath != *output.ContainerPath {
-		t.Errorf("Expected containerPath [%s] But was [%s]", containerPath, *output.ContainerPath)
-	}
-	if source == "" {
-		sourceVolume = volumes.VolumeEmptyHost[EmptyHostCtr]
-	} else if project.IsNamedVolume(source) {
-		sourceVolume = source
-	} else {
-		sourceVolume = volumes.VolumeWithHost[source]
+func TestConvertToMountPointsWithNoCorrespondingNamedVolume(t *testing.T) {
+	volumes := &Volumes{
+		VolumeWithHost:  make(map[string]string),
+		VolumeEmptyHost: []string{}, // Top-level named volumes is empty
 	}
-	if sourceVolume != *output.SourceVolume {
-		t.Errorf("Expected sourceVolume [%s] But was [%s]", sourceVolume, *output.SourceVolume)
+
+	namedVolume := yaml.Volume{
+		Source:      namedVolume,
+		Destination: containerPath,
 	}
-	if readonly != *output.ReadOnly {
-		t.Errorf("Expected readonly [%v] But was [%v]", readonly, *output.ReadOnly)
+
+	mountPointsIn := yaml.Volumes{
+		Volumes: []*yaml.Volume{&namedVolume},
 	}
+
+	_, err := ConvertToMountPoints(&mountPointsIn, volumes)
+	assert.Error(t, err, "Expected error converting MountPoints")
 }
 
 func TestConvertToExtraHosts(t *testing.T) {
diff --git a/ecs-cli/modules/cli/compose/project/mock/project.go b/ecs-cli/modules/cli/compose/project/mock/project.go
@@ -186,3 +186,13 @@ func (_m *MockProject) Up() error {
 func (_mr *_MockProjectRecorder) Up() *gomock.Call {
 	return _mr.mock.ctrl.RecordCall(_mr.mock, "Up")
 }
+
+func (_m *MockProject) VolumeConfigs() *adapter.Volumes {
+	ret := _m.ctrl.Call(_m, "VolumeConfigs")
+	ret0, _ := ret[0].(*adapter.Volumes)
+	return ret0
+}
+
+func (_mr *_MockProjectRecorder) VolumeConfigs() *gomock.Call {
+	return _mr.mock.ctrl.RecordCall(_mr.mock, "VolumeConfigs")
+}
diff --git a/ecs-cli/modules/cli/compose/project/project.go b/ecs-cli/modules/cli/compose/project/project.go
@@ -38,6 +38,7 @@ type Project interface {
 	Context() *context.ECSContext
 	ServiceConfigs() *config.ServiceConfigs
 	ContainerConfigs() []adapter.ContainerConfig // TODO change this to a pointer to slice?
+	VolumeConfigs() *adapter.Volumes
 	Entity() entity.ProjectEntity
 
 	// commands
@@ -57,6 +58,7 @@ type ecsProject struct {
 
 	// TODO: use this instead of project.Project
 	containerConfigs []adapter.ContainerConfig
+	volumes          *adapter.Volumes
 
 	ecsContext *context.ECSContext
 
@@ -100,8 +102,8 @@ func (p *ecsProject) ServiceConfigs() *config.ServiceConfigs {
 }
 
 // VolumeConfigs returns a map of Volume Configuration loaded from compose yaml file
-func (p *ecsProject) VolumeConfigs() map[string]*config.VolumeConfig {
-	return p.Project.VolumeConfigs
+func (p *ecsProject) VolumeConfigs() *adapter.Volumes {
+	return p.volumes
 }
 
 func (p *ecsProject) ContainerConfigs() []adapter.ContainerConfig {
@@ -138,7 +140,7 @@ func (p *ecsProject) Parse() error {
 	return p.transformTaskDefinition()
 }
 
-// parseCompose sets data from the compose files on the ecsProject
+// parseCompose sets data from the compose files on the ecsProject, including ContainerConfigs and VolumeConfigs
 func (p *ecsProject) parseCompose() error {
 	logrus.Debug("Parsing the compose yaml...")
 
@@ -153,6 +155,7 @@ func (p *ecsProject) parseCompose() error {
 		if err != nil {
 			return err
 		}
+		// TODO: set this in parseV1V2 itself?
 		p.containerConfigs = *configs
 	case "3", "3.0":
 		configs, err := p.parseV3()
@@ -197,7 +200,7 @@ func (p *ecsProject) transformTaskDefinition() error {
 
 	taskDefinition, err := utils.ConvertToTaskDefinition(
 		&ecsContext.Context,
-		p.VolumeConfigs(),
+		p.VolumeConfigs(),    // will have already been transformed to adapter.Volumes in parseCompose
 		p.ContainerConfigs(), // TODO Change to pointer on project?
 		taskRoleArn,
 		requiredCompatibilities,
diff --git a/ecs-cli/modules/cli/compose/project/project_parseV1V2.go b/ecs-cli/modules/cli/compose/project/project_parseV1V2.go
@@ -16,6 +16,8 @@ func (p *ecsProject) parseV1V2() (*[]adapter.ContainerConfig, error) {
 	// context. It sets up the name, the composefile and the composebytes
 	// (the composefile content). This is where libcompose ServiceConfigs
 	// and VolumeConfigs gets loaded.
+
+	// TODO instantiate the libcompose project here instead of in the factory
 	if err := p.Project.Parse(); err != nil {
 		return nil, err
 	}
@@ -25,6 +27,7 @@ func (p *ecsProject) parseV1V2() (*[]adapter.ContainerConfig, error) {
 	if err != nil {
 		return nil, err
 	}
+	p.volumes = volumes
 
 	context := &p.Context().Context
 	serviceConfigs := p.Project.ServiceConfigs
diff --git a/ecs-cli/modules/cli/compose/project/project_parseV1V2_test.go b/ecs-cli/modules/cli/compose/project/project_parseV1V2_test.go
@@ -213,19 +213,29 @@ func TestParseV1V2_Version2Files(t *testing.T) {
 	memory := int64(512)
 	mountPoints := []*ecs.MountPoint{
 		{
-			ContainerPath: aws.String("/tmp/cache"),
+			ContainerPath: aws.String("/var/lib/mysql"),
 			ReadOnly:      aws.Bool(false),
-			SourceVolume:  aws.String("banana"),
+			SourceVolume:  aws.String("volume-1"),
 		},
 		{
-			ContainerPath: aws.String("/tmp/cache"),
+			ContainerPath: aws.String("/var/lib/mysql"),
 			ReadOnly:      aws.Bool(false),
-			SourceVolume:  aws.String("volume-1"),
+			SourceVolume:  aws.String("volume-2"),
 		},
 		{
 			ContainerPath: aws.String("/tmp/cache"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("volume-3"),
+		},
+		{
+			ContainerPath: aws.String("/etc/configs/"),
 			ReadOnly:      aws.Bool(true),
-			SourceVolume:  aws.String("volume-2"),
+			SourceVolume:  aws.String("volume-4"),
+		},
+		{
+			ContainerPath: aws.String("/var/lib/mysql"),
+			ReadOnly:      aws.Bool(false),
+			SourceVolume:  aws.String("datavolume"),
 		},
 	}
 	ports := []*ecs.PortMapping{
@@ -279,11 +289,13 @@ services:
   mysql:
     image: mysql
     volumes:
-      - banana:/tmp/cache
-      - :/tmp/cache
-      - ./cache:/tmp/cache:ro
+      - /var/lib/mysql
+      - /opt/data:/var/lib/mysql
+      - ./cache:/tmp/cache:rw
+      - ~/configs:/etc/configs/:ro
+      - datavolume:/var/lib/mysql
 volumes:
-  banana:`
+  datavolume:`
 
 	// Setup docker-compose file
 	tmpfile, err := ioutil.TempFile("", "test")
@@ -326,7 +338,7 @@ volumes:
 	assert.NoError(t, err, "Unexpected error retrieving wordpress config")
 
 	assert.Equal(t, mysqlImage, mysql.Image, "Expected mysql Image to match")
-	assert.Equal(t, mountPoints, mysql.MountPoints, "Expected MountPoints to match")
+	assert.ElementsMatch(t, mountPoints, mysql.MountPoints, "Expected MountPoints to match")
 }
 
 func TestParseV1V2_Version1_WithEnvFile(t *testing.T) {
diff --git a/ecs-cli/modules/cli/compose/project/project_test.go b/ecs-cli/modules/cli/compose/project/project_test.go
@@ -63,6 +63,42 @@ services:
 	assert.Equal(t, testProjectName, project.ecsContext.ProjectName, "Expected ProjectName to be overridden.")
 }
 
+func TestParseCompose_V2_WithVolumeConfigs(t *testing.T) {
+	// Setup docker-compose file
+	composeFileString := `version: '2'
+services:
+  wordpress:
+    image: wordpress
+  mysql:
+    image: mysql
+    volumes:
+      - banana:/tmp/cache
+      - :/tmp/cache
+      - ./cache:/tmp/cache:ro
+volumes:
+  banana:`
+
+	tmpfile, err := ioutil.TempFile("", "test")
+	assert.NoError(t, err, "Unexpected error in creating test file")
+
+	defer os.Remove(tmpfile.Name())
+
+	_, err = tmpfile.Write([]byte(composeFileString))
+	assert.NoError(t, err, "Unexpected error in writing to test file")
+
+	err = tmpfile.Close()
+	assert.NoError(t, err, "Unexpected error closing file")
+
+	// Set up project
+	project := setupTestProject(t)
+	project.ecsContext.ComposeFiles = append(project.ecsContext.ComposeFiles, tmpfile.Name())
+
+	err = project.parseCompose()
+
+	// verify VolumeConfigs are populated
+	assert.NotEmpty(t, project.VolumeConfigs(), "Expected volume configs to not be empty")
+}
+
 func TestParseECSParams(t *testing.T) {
 	ecsParamsString := `version: 1
 task_definition:
diff --git a/ecs-cli/modules/utils/compose/convert_task_definition.go b/ecs-cli/modules/utils/compose/convert_task_definition.go
@@ -87,7 +87,7 @@ type TaskDefParams struct {
 
 // ConvertToTaskDefinition transforms the yaml configs to its ecs equivalent (task definition)
 // TODO container config a pointer to slice?
-func ConvertToTaskDefinition(context *project.Context, volumeConfigs map[string]*config.VolumeConfig,
+func ConvertToTaskDefinition(context *project.Context, volumes *adapter.Volumes,
 	containerConfigs []adapter.ContainerConfig, taskRoleArn string, requiredCompatibilites string, ecsParams *ECSParams) (*ecs.TaskDefinition, error) {
 	if len(containerConfigs) == 0 {
 		return nil, errors.New("cannot create a task definition with no containers; invalid service config")
@@ -108,12 +108,6 @@ func ConvertToTaskDefinition(context *project.Context, volumeConfigs map[string]
 		taskRoleArn = taskDefParams.taskRoleArn
 	}
 
-	// TODO: Refactor when Volumes added to top level project
-	volumes, err := adapter.ConvertToVolumes(volumeConfigs)
-	if err != nil {
-		return nil, err
-	}
-
 	// Create containerDefinitions
 	containerDefinitions := []*ecs.ContainerDefinition{}
 
diff --git a/ecs-cli/modules/utils/compose/convert_task_definition_test.go b/ecs-cli/modules/utils/compose/convert_task_definition_test.go
