[PLAT-71] Allow define one optional volume #3

keymon · 2017-05-25T15:45:18Z

Note: same as #2 fixing spurious content

For some cases and workloads we need to pass some modules to the task definition. The volumes are passed as a list of maps to the ecs_task_resource[1].

But the current syntax of terraform does not allow consume this value directly from a variable. Trying to do so, we hit the issues described in [2] and [3] (pending to report a specific bug).

But we found out that it works if we build the map structure directly within the resource, and we use interpolation to consume the values of the map.

Meanwhile the terraform project does not provide a definitive solution, we
will implement the following workaround:

The module will receive configuration for one unique module, being the default a empty map {}
If the map is empty, a dummy volume will be passed as name=dummy and host_path=/tmp/dummy_volume

This would cover our specific case in the short term, and can be later easily adapted to use a optional list of volumes.

[1] https://www.terraform.io/docs/providers/aws/r/ecs_task_definition.html#volume
[2] hashicorp/terraform#10407
[3] hashicorp/terraform#7705 (comment)

For some cases and workloads we need to pass some modules to the task definition. The volumes are passed as a list of maps to the ecs_task_resource[1]. But the current syntax of terraform does not allow consume this value directly from a variable. Trying to do so, we hit the issues described in [2] and [3] (pending to report a specific bug). But we found out that it works if we build the map structure directly within the resource, and we use interpolation to consume the values of the map. Meanwhile the terraform project does not provide a definitive solution, we will implement the following workaround: - The module will receive configuration for one unique module, being the default a empty map {} - If the map is empty, a dummy volume will be passed as `name=dummy` and `host_path=/tmp/dummy_volume` This would cover our specific case in the short term, and can be later easily adapted to use a optional list of volumes. [1] https://www.terraform.io/docs/providers/aws/r/ecs_task_definition.html#volume [2] hashicorp/terraform#10407 [3] hashicorp/terraform#7705 (comment)

Pass the volume variable added in [1] [1] mergermarket/tf_ecs_task_definition#3

Meanwhile [1] is not merge, point to that branch to allow review and test this PR. Revert once [1] is merged. [1] mergermarket/tf_ecs_task_definition#3

In order to avoid creating spurious files in the current directory (.terraform and __pycache__) we run terraform in a temporary directory within the container and we disable python bytecode generation by setting PYTHONDONTWRITEBYTECODE env var. https://docs.python.org/2/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE Also cleanup test code

bart613

Some small bits for your consideration to tweak. Otherwise solid stuff 👍

bart613 · 2017-06-01T09:11:47Z

README.md

@@ -23,6 +23,8 @@ This module creates a basic ECS Task Definition.

 * `family` - the name of the task definition. For ECS services it is recommended to use the same name as for the service, and for that name to consist of the environment name (e.g. "live"), the comonent name (e.g. "foobar-service"), and an optional suffix (if an environment has multiple services for the component running - e.g. in a multi-tenant setup), separated by hyphens.
 * `container_definitions` - list of strings. Each string should be a JSON document describing a single container definition - see https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html.
+* `task_role_arn` - The Amazon Resource Name for an IAM role for the task.
+* `volume` - Volume block map with 'name' and 'host_path'. 'name': The name of the volume as is referenced in the sourceVolume. 'host_path' The path on the host container instance that is presented to the container.


This, again, I'd consider shortening description here and refer to full documentation with example

bart613 · 2017-06-01T09:13:04Z

test/infra/main.tf

@@ -16,11 +16,18 @@ variable "task_role_arn_param" {
    default = ""
 }

+variable "task_volume_param" {
+    description = "Allow the test to pass this in"


bart613 · 2017-06-01T09:13:21Z

test/infra/main.tf

+    type = "map"
+    default = {}
+}
+
 module "taskdef" {
  source = "../.."

  family = "tf_ecs_taskdef_test_family"


terraform fmt this bit of code please

bart613 · 2017-06-01T09:13:34Z

test/test_taskdef.py

@@ -1,32 +1,45 @@
-import re
 import unittest


bart613 · 2017-06-01T09:13:52Z

test/test_taskdef.py

+        self.module_path = os.path.join(os.getcwd(), 'test', 'infra')
+
+        check_call(
+            ['terraform', 'get', self.module_path],


Would this not fit on a single line?

bart613 · 2017-06-01T09:15:08Z

variables.tf

@@ -13,3 +13,9 @@ variable "task_role_arn" {
    type = "string"
    default = ""
 }
+
+variable "volume" {
+    description = "Volume block map with 'name' and 'host_path'. 'name': The name of the volume as is referenced in the sourceVolume. 'host_path' The path on the host container instance that is presented to the container."


Consider shortening description and refer to README

Make the description shorten and refer to the original documentation.

keymon · 2017-06-01T09:41:07Z

@bart613 👍 thx, I applied the suggested changes

For some cases and workloads we need to pass some mountpoints to the container definition. The mountpoints are passed as a list of maps in the json under `mountPoints`[1]. In the related modules tf_task_definition, we added logic to define one optional volume[2], and here we allow to specify the mounpoint for that volume. The mountpoint is defined as a map with the keys `sourceVolume`, `containerPath` and (optional) `readOnly` . [1] http://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html#container_definition_storage [2] mergermarket/tf_ecs_task_definition#3

keymon added 2 commits May 25, 2017 12:53

Add missing param task_role_arn in the README

7e12816

keymon added a commit to mergermarket/tf_ecs_task_definition_with_task_role that referenced this pull request May 25, 2017

Pass volume variable to task_definition module

c80ae18

Pass the volume variable added in [1] [1] mergermarket/tf_ecs_task_definition#3

keymon mentioned this pull request May 25, 2017

[PLAT-71] Pass volume variable to tf_ecs_task_definition mergermarket/tf_ecs_task_definition_with_task_role#1

Merged

bart613 suggested changes Jun 1, 2017

View reviewed changes

keymon added 3 commits June 1, 2017 10:34

Improve documentation of variables

3b81db0

Make the description shorten and refer to the original documentation.

Run terraform fmt for all the code

a74de89

Apply isort to test file

939ebb8

bart613 approved these changes Jun 1, 2017

View reviewed changes

keymon merged commit 5146979 into master Jun 1, 2017

keymon deleted the PLAT-71_add_volume_2 branch June 1, 2017 11:30

keymon mentioned this pull request Jun 6, 2017

PLAT-71: Allow define one optional volume mergermarket/tf_ecs_container_definition#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLAT-71] Allow define one optional volume #3

[PLAT-71] Allow define one optional volume #3

keymon commented May 25, 2017 •

edited

Loading

bart613 left a comment

bart613 Jun 1, 2017

bart613 Jun 1, 2017

bart613 Jun 1, 2017

bart613 Jun 1, 2017

bart613 Jun 1, 2017

bart613 Jun 1, 2017

keymon commented Jun 1, 2017

[PLAT-71] Allow define one optional volume #3

[PLAT-71] Allow define one optional volume #3

Conversation

keymon commented May 25, 2017 • edited Loading

bart613 left a comment

Choose a reason for hiding this comment

bart613 Jun 1, 2017

Choose a reason for hiding this comment

bart613 Jun 1, 2017

Choose a reason for hiding this comment

bart613 Jun 1, 2017

Choose a reason for hiding this comment

bart613 Jun 1, 2017

Choose a reason for hiding this comment

bart613 Jun 1, 2017

Choose a reason for hiding this comment

bart613 Jun 1, 2017

Choose a reason for hiding this comment

keymon commented Jun 1, 2017

keymon commented May 25, 2017 •

edited

Loading