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

Fixes #11: Error attempting to ecs-cli compose up #201

Merged
merged 2 commits into from
Mar 10, 2017
Merged

Fixes #11: Error attempting to ecs-cli compose up #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8aef474
Don't create Host key in Volume if no hostPath is defined
mcanevet Oct 14, 2015
71b070a
Fixes #11: Creates new volume when there's no host path
tiffanyfay Feb 9, 2017
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
47 changes: 34 additions & 13 deletions ecs-cli/modules/compose/ecs/utils/convert_task_definition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ var supportedComposeYamlOptions = []string{

var supportedComposeYamlOptionsMap = getSupportedComposeYamlOptionsMap()

type volumes struct {
volumeWithHost map[string]string
volumeEmptyHost []string
}

func getSupportedComposeYamlOptionsMap() map[string]bool {
optionsMap := make(map[string]bool)
for _, value := range supportedComposeYamlOptions {
Expand All @@ -67,7 +72,9 @@ func ConvertToTaskDefinition(taskDefinitionName string, context *project.Context
logUnsupportedConfigFields(context.Project)

containerDefinitions := []*ecs.ContainerDefinition{}
volumes := make(map[string]string) // map with key:=hostSourcePath value:=VolumeName
volumes := &volumes{
volumeWithHost: make(map[string]string), // map with key:=hostSourcePath value:=VolumeName
}

for _, name := range serviceConfigs.Keys() {
serviceConfig, ok := serviceConfigs.Get(name)
Expand All @@ -83,6 +90,7 @@ func ConvertToTaskDefinition(taskDefinitionName string, context *project.Context
}
containerDefinitions = append(containerDefinitions, containerDef)
}

taskDefinition := &ecs.TaskDefinition{
Family: aws.String(taskDefinitionName),
ContainerDefinitions: containerDefinitions,
Expand Down Expand Up @@ -156,7 +164,7 @@ func isZero(v reflect.Value) bool {
// convertToContainerDef transforms each service in the compose yml
// to an equivalent container definition
func convertToContainerDef(context *project.Context, inputCfg *config.ServiceConfig,
volumes map[string]string, outputContDef *ecs.ContainerDefinition) error {
volumes *volumes, outputContDef *ecs.ContainerDefinition) error {
// setting memory
var mem int64
if inputCfg.MemLimit != 0 {
Expand Down Expand Up @@ -288,14 +296,21 @@ func createKeyValuePair(key, value string) *ecs.KeyValuePair {
}

// convertToECSVolumes transforms the map of hostPaths to the format of ecs.Volume
func convertToECSVolumes(hostPaths map[string]string) []*ecs.Volume {
func convertToECSVolumes(hostPaths *volumes) []*ecs.Volume {
output := []*ecs.Volume{}
for hostPath, volName := range hostPaths {
// volumes with a host path
for hostPath, volName := range hostPaths.volumeWithHost {
ecsVolume := &ecs.Volume{
Name: aws.String(volName),
Host: &ecs.HostVolumeProperties{
SourcePath: aws.String(hostPath),
},
}}
output = append(output, ecsVolume)
}
// volumes with an empty host path
for _, volName := range hostPaths.volumeEmptyHost {
ecsVolume := &ecs.Volume{
Name: aws.String(volName),
}
output = append(output, ecsVolume)
}
Expand Down Expand Up @@ -420,7 +435,7 @@ func convertToVolumesFrom(cfgVolumesFrom []string) ([]*ecs.VolumeFrom, error) {

// convertToMountPoints transforms the yml volumes slice to ecs compatible MountPoints slice
// It also uses the hostPath from volumes if present, else adds one to it
func convertToMountPoints(cfgVolumes *yaml.Volumes, volumes map[string]string) ([]*ecs.MountPoint, error) {
func convertToMountPoints(cfgVolumes *yaml.Volumes, volumes *volumes) ([]*ecs.MountPoint, error) {
mountPoints := []*ecs.MountPoint{}
if cfgVolumes == nil {
return mountPoints, nil
Expand All @@ -443,13 +458,19 @@ func convertToMountPoints(cfgVolumes *yaml.Volumes, volumes map[string]string) (
}

var volumeName string
if len(volumes) > 0 {
volumeName = volumes[hostPath]
}

if volumeName == "" {
volumeName = getVolumeName(len(volumes))
volumes[hostPath] = volumeName
numVol := len(volumes.volumeWithHost) + len(volumes.volumeEmptyHost)
if hostPath == "" {
// add mount point for volumes with an empty host path
volumeName = getVolumeName(numVol)
volumes.volumeEmptyHost = append(volumes.volumeEmptyHost, volumeName)
} else {
// add mount point for volumes with a host path
volumeName = volumes.volumeWithHost[hostPath]

if volumeName == "" {
volumeName = getVolumeName(numVol)
volumes.volumeWithHost[hostPath] = volumeName
}
}

mountPoints = append(mountPoints, &ecs.MountPoint{
Expand Down
58 changes: 40 additions & 18 deletions ecs-cli/modules/compose/ecs/utils/convert_task_definition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ import (
)

const (
portNumber = 8000
portMapping = "8000:8000"
containerPath = "/tmp/cache"
hostPath = "./cache"
portNumber = 8000
portMapping = "8000:8000"
containerPath = "/tmp/cache"
containerPath2 = "/tmp/cache2"
hostPath = "./cache"
)

func TestConvertToTaskDefinition(t *testing.T) {
Expand Down Expand Up @@ -262,7 +263,7 @@ func TestConvertToTaskDefinitionWithLogConfiguration(t *testing.T) {
taskDefinition = convertToTaskDefinitionInTest(t, "name", serviceConfig)
containerDef = *taskDefinition.ContainerDefinitions[0]
if logDriver != aws.StringValue(containerDef.LogConfiguration.LogDriver) {
t.Errorf("Expected Log driver [%s]. But was [%s]", containerDef.LogConfiguration)
t.Errorf("Expected Log driver [%s]. But was [%s]", logDriver, aws.StringValue(containerDef.LogConfiguration.LogDriver))
}
if !reflect.DeepEqual(logOpts, aws.StringValueMap(containerDef.LogConfiguration.Options)) {
t.Errorf("Expected Log options [%v]. But was [%v]", logOpts, aws.StringValueMap(containerDef.LogConfiguration.Options))
Expand Down Expand Up @@ -350,14 +351,19 @@ func verifyPortMapping(t *testing.T, output *ecs.PortMapping, hostPort, containe

func TestConvertToMountPoints(t *testing.T) {
onlyContainerPath := yaml.Volume{Destination: containerPath}
hostAndContainerPath := yaml.Volume{Source: hostPath, Destination: containerPath} // "./cache:/tmp/cache"
onlyContainerPath2 := yaml.Volume{Destination: containerPath2}
hostAndContainerPath := yaml.Volume{Source: hostPath, Destination: containerPath} // "./cache:/tmp/cache"
onlyContainerPathWithRO := yaml.Volume{Destination: containerPath, AccessMode: "ro"}
hostAndContainerPathWithRO := yaml.Volume{Source: hostPath, Destination: containerPath, AccessMode: "ro"} // "./cache:/tmp/cache:ro"
hostAndContainerPathWithRW := yaml.Volume{Source: hostPath, Destination: containerPath, AccessMode: "rw"}

volumes := make(map[string]string)
volumes := &volumes{
volumeWithHost: make(map[string]string), // map with key:=hostSourcePath value:=VolumeName
}

mountPointsIn := yaml.Volumes{Volumes: []*yaml.Volume{&onlyContainerPath, &hostAndContainerPath,
&hostAndContainerPathWithRO, &hostAndContainerPathWithRW}}
// Valid inputs with host and container paths set
mountPointsIn := yaml.Volumes{Volumes: []*yaml.Volume{&onlyContainerPath, &onlyContainerPath2, &hostAndContainerPath,
&onlyContainerPathWithRO, &hostAndContainerPathWithRO, &hostAndContainerPathWithRW}}

mountPointsOut, err := convertToMountPoints(&mountPointsIn, volumes)
if err != nil {
Expand All @@ -366,11 +372,23 @@ func TestConvertToMountPoints(t *testing.T) {
if len(mountPointsIn.Volumes) != len(mountPointsOut) {
t.Errorf("Incorrect conversion. Input [%v] Output [%v]", mountPointsIn, mountPointsOut)
}
verifyMountPoint(t, mountPointsOut[0], volumes, "", containerPath, false)
verifyMountPoint(t, mountPointsOut[1], volumes, hostPath, containerPath, false)
verifyMountPoint(t, mountPointsOut[2], volumes, hostPath, containerPath, true)
verifyMountPoint(t, mountPointsOut[3], volumes, hostPath, containerPath, false)

verifyMountPoint(t, mountPointsOut[0], volumes, "", containerPath, false, 0) // 0 is the counter for the first volume with an empty host path
verifyMountPoint(t, mountPointsOut[1], volumes, "", containerPath2, false, 1) // 1 is the counter for the second volume with an empty host path
verifyMountPoint(t, mountPointsOut[2], volumes, hostPath, containerPath, false, 1)
verifyMountPoint(t, mountPointsOut[3], volumes, "", containerPath, true, 2) // 2 is the counter for the third volume with an empty host path
verifyMountPoint(t, mountPointsOut[4], volumes, hostPath, containerPath, true, 2)
verifyMountPoint(t, mountPointsOut[5], volumes, hostPath, containerPath, false, 2)

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

if mountPointsOut[1].SourceVolume == mountPointsOut[3].SourceVolume {
t.Errorf("Expected volume %v (onlyContainerPath2) and %v (onlyContainerPathWithRO) to be different", mountPointsOut[0].SourceVolume, mountPointsOut[1].SourceVolume)
}

// Invalid access mode input
hostAndContainerPathWithIncorrectAccess := yaml.Volume{Source: hostPath, Destination: containerPath, AccessMode: "readonly"}
mountPointsIn = yaml.Volumes{Volumes: []*yaml.Volume{&hostAndContainerPathWithIncorrectAccess}}
mountPointsOut, err = convertToMountPoints(&mountPointsIn, volumes)
Expand All @@ -387,18 +405,22 @@ func TestConvertToMountPoints(t *testing.T) {
}
}

func verifyMountPoint(t *testing.T, output *ecs.MountPoint, volumes map[string]string,
hostPath, containerPath string, readonly bool) {

func verifyMountPoint(t *testing.T, output *ecs.MountPoint, volumes *volumes,
hostPath, containerPath string, readonly bool, EmptyHostCtr int) {
sourceVolume := ""
if containerPath != *output.ContainerPath {
t.Errorf("Expected containerPath [%s] But was [%s]", containerPath, *output.ContainerPath)
}
sourceVolume := volumes[hostPath]
if hostPath != "" {
sourceVolume = volumes.volumeWithHost[hostPath]
} else {
sourceVolume = volumes.volumeEmptyHost[EmptyHostCtr]
}
if sourceVolume != *output.SourceVolume {
t.Errorf("Expected sourceVolume [%s] But was [%s]", sourceVolume, *output.SourceVolume)
}
if readonly != *output.ReadOnly {
t.Errorf("Expected readonly [%s] But was [%s]", readonly, *output.ReadOnly)
t.Errorf("Expected readonly [%v] But was [%v]", readonly, *output.ReadOnly)
Copy link

Choose a reason for hiding this comment

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

Have you tried using testify/assert package for these kinds of things? Makes it less verbose and more elegant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't because that would require rewriting the entire test file or just changing the section I was dealing with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding testify/assert will be a separate PR since it's not specific to this issue and isn't currently used in the file.

}
}

Expand Down