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

Add support for credentials_parameter #573

Merged
merged 2 commits into from
Aug 3, 2018
Merged

Add support for credentials_parameter #573

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
1f8c534
Add credentials_parameter to ecs-params
allisaurus Jul 3, 2018
681a036
Add credentials_parameter to task definition
allisaurus Jul 5, 2018
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
8 changes: 8 additions & 0 deletions ecs-cli/modules/utils/compose/convert_task_definition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,16 @@ func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *C
mem = resolveIntResourceOverride(inputCfg.Name, mem, ecsMemInMB, "MemoryLimit")

ecsMemResInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.MemoryReservation))

memRes = resolveIntResourceOverride(inputCfg.Name, memRes, ecsMemResInMB, "MemoryReservation")

credParam := ecsContainerDef.RepositoryCredentials.CredentialsParameter

if credParam != "" {
outputContDef.RepositoryCredentials = &ecs.RepositoryCredentials{}
Copy link
Contributor

Choose a reason for hiding this comment

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

This it totally fine, but just wondering aloud: We are sometimes inconsistent with using the setters on objects vs just assigning the field to an empty version of it. E.g. we're using SetCredentialsParameter in line 185, but not SetRepositoryCredentials in line 184. Is this something we might want to think about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, we are inconsistent with this. But they're each more concise depending on what you want to do.

In the case of setting up a RepositoryCredentials, if I wanted to use the setter method I'd have to create an empty struct first, and then set it with the method (otherwise it'd set it to an empty pointer, so I wouldn't have a thing to call SetCredentialsParameter on), so setting directly like on line 184 just lets me do it in one line. On 185 the value I want to set already exists, so I use the setter method, which also takes care of converting it to a pointer so I don't have to do aws.String().

This isn't definitive guidance of course, but perhaps using either depending on what results in fewer lines of code might be one way of deciding?

outputContDef.RepositoryCredentials.SetCredentialsParameter(credParam)
}

var err error
healthCheck, err = resolveHealthCheck(inputCfg.Name, healthCheck, ecsContainerDef.HealthCheck)
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion ecs-cli/modules/utils/compose/convert_task_definition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,8 @@ task_definition:
services:
mysql:
essential: false
repository_credentials:
credentials_parameter: arn:aws:secretsmanager:1234567890:secret:test-secret
task_size:
mem_limit: 5Gb
cpu_limit: 256`
Expand Down Expand Up @@ -471,7 +473,7 @@ task_definition:
assert.Equal(t, "256", aws.StringValue(taskDefinition.Cpu), "Expected CPU to match")
assert.Equal(t, "5Gb", aws.StringValue(taskDefinition.Memory), "Expected CPU to match")
assert.True(t, aws.BoolValue(wordpress.Essential), "Expected container with name: '%v' to be true", *wordpress.Name)

assert.Equal(t, "arn:aws:secretsmanager:1234567890:secret:test-secret", aws.StringValue(mysql.RepositoryCredentials.CredentialsParameter), "Expected CredentialsParameter to match")
}
}

Expand Down
8 changes: 7 additions & 1 deletion ecs-cli/modules/utils/compose/ecs_params_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ type ContainerDefs map[string]ContainerDef

// ContainerDef holds fields for an ECS Container Definition that are not supplied by docker-compose
type ContainerDef struct {
Essential bool `yaml:"essential"`
Essential bool `yaml:"essential"`
RepositoryCredentials RepositoryCredentials `yaml:"repository_credentials"`
// resource field yaml names correspond to equivalent docker-compose field
Cpu int64 `yaml:"cpu_shares"`
Memory libYaml.MemStringorInt `yaml:"mem_limit"`
Expand All @@ -70,6 +71,11 @@ type HealthCheck struct {
StartPeriod string `yaml:"start_period,omitempty"`
}

// RepositoryCredentials holds CredentialParameters for a ContainerDef
type RepositoryCredentials struct {
CredentialsParameter string `yaml:"credentials_parameter"`
}

// TaskSize holds Cpu and Memory values needed for Fargate tasks
// https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-cpu-memory-error.html
type TaskSize struct {
Expand Down
5 changes: 4 additions & 1 deletion ecs-cli/modules/utils/compose/ecs_params_reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ task_definition:
mem_limit: 524288000
mem_reservation: 500mb
wordpress:
essential: true`
essential: true
repository_credentials:
credentials_parameter: arn:aws:secretsmanager:1234567890:secret:test-RT4iv`

content := []byte(ecsParamsString)

Expand Down Expand Up @@ -117,6 +119,7 @@ task_definition:
assert.Equal(t, yaml.MemStringorInt(524288000), mysql.Memory)
assert.Equal(t, yaml.MemStringorInt(524288000), mysql.MemoryReservation)
assert.True(t, wordpress.Essential, "Expected container to be essential")
assert.Equal(t, "arn:aws:secretsmanager:1234567890:secret:test-RT4iv", wordpress.RepositoryCredentials.CredentialsParameter, "Expected CredentialsParameter to match")
}
}

Expand Down