Added support to specify FSx Windows file server volumes #166
Conversation
…task_definition resource
|
/test all |
|
@nitrocode / @Gowiem / @joe-niland ... can anyone have a look at this PR. |
|
/test all |
There was a problem hiding this comment.
@gauravkohli This looks solid to me -- Well done.
Before we merge, I would like a second set of eyes on this code since it's so deeply nested. @nitrocode or @joe-niland mind putting eyes on this?
There was a problem hiding this comment.
@Gowiem it looks logically correct. I don't think we have an easy way of testing it with the current setup as it will require creating a Windows ECS task.
variables.tf
Outdated
| @@ -279,7 +279,7 @@ variable "efs_volumes" { | |||
| })) | |||
| })) | |||
|
|
|||
| description = "Task EFS volume definitions as list of configuration objects. You cannot define both Docker volumes and EFS volumes on the same task definition." | |||
| description = "Task EFS volume definitions as list of configuration objects. You can define only one of Docker volumes, EFS volumes and FSx volumes on the same task definition." | |||
There was a problem hiding this comment.
@gauravkohli just wanted to clarify the var description: when you say "one of" does that mean "one of each type" or "one type"? It seems mixing types is supported (just based on the AWS docs)
There was a problem hiding this comment.
@joe-niland .. comment change was just following the old comment which looks like meant we can only have either Docker volumes or EFS volumes on the same task definition. So I added we can only have one type, but multiple volumes of the same type.
But then I tested it on the ECS task and actually as you said we can indeed mix types, so we can have a task definition that can have all Docker, EFS & Fsx volumes attached to it and AWS doesn't complain about it.
There was a problem hiding this comment.
maybe useful info to add would be that the list fsx_windows_file_server_volume_configuration can only have one element, as that list is used for the dynamic volume block, and each volume block can only have one of fsx_windows_file_server_volume_configuration
There was a problem hiding this comment.
@gauravkohli that sounds like a good change, as well as perhaps changing 'only one of' to 'multiple'?
There was a problem hiding this comment.
@joe-niland .. implemented the suggestion. Can we go ahead and merge this PR now?
|
/test all |
|
Thanks for your contribution @gauravkohli - released |
…alb-service-task * 'master' of github.com:SevenPicoForks/terraform-aws-ecs-alb-service-task: fixes: Changing task_exec_policy_arns or task_policy_arns cause recreations cloudposse#167 (cloudposse#178) make `host_path` optional for fargate (cloudposse#176) Adding support for bind mount volume types (cloudposse#173) Update README.md and docs (cloudposse#171) Set bool inputs to type bool (cloudposse#170) Add ecs_service_enabled (cloudposse#169) BUG | Support Dynamic deployment_circuit_breaker for CODE_DEPLOY (cloudposse#165) Added support to specify FSx Windows file server volumes (cloudposse#166) git-xargs programmatic commit (cloudposse#163) # Conflicts: # main.tf
what
Docker/EFSvolumes to the ECS task definition. With this PR added support to addFSx Windows file servervolumes to the ECS task definitions.why
references