telia-oss / terraform-aws-ecs-fargate

Terraform module which creates Fargate ECS resources on AWS.
https://registry.terraform.io/modules/telia-oss/ecs-fargate/aws
MIT License
82 stars 75 forks source link

EFS volumes #53

Closed feraudet closed 2 years ago

feraudet commented 3 years ago

Add:

larstobi commented 3 years ago

Thanks for the PR!

Have you tested it on a clean branch?

I wonder if a security group and rule for EFS might be needed? Like specified in #43.

Also, does it not need a mountPoints entry in the container definition?

feraudet commented 3 years ago

Security-group is not needed with fargate thanks to the IAM authorization. But yes, the mount point is missing, I will fix it then test it on a fresh branch

feraudet commented 3 years ago
larstobi commented 3 years ago

Very nice, @feraudet !

Did you see my comment on the variable specification for efs_volumes ?

feraudet commented 3 years ago

I did not see it. Here an additional commit with specs

larstobi commented 2 years ago

Nice work, @feraudet !

Is the change to make lb_arns optional necessary for EFS support? If it's not necessary, I think it should be taken out and added in a separate PR. We do want the optional lb_arns feature, though! I've use for it myself, and have done the same with a local copy when needed. And please add a new folder in the examples directory with a name like "no-lb" or similar, where the example with no lb is shown.

We would like to keep the basic example as basic as possible. Please copy the basic folder in the example directory to a new folder called "efs" and check it in, and revert the basic example.

Otherwise, it looks great and soon ready for merge.

larstobi commented 2 years ago

The target group fix can be removed in this PR in favour of #59

feraudet commented 2 years ago

Nice work, @feraudet !

Is the change to make lb_arns optional necessary for EFS support? If it's not necessary, I think it should be taken out and added in a separate PR. We do want the optional lb_arns feature, though! I've use for it myself, and have done the same with a local copy when needed. And please add a new folder in the examples directory with a name like "no-lb" or similar, where the example with no lb is shown.

We would like to keep the basic example as basic as possible. Please copy the basic folder in the example directory to a new folder called "efs" and check it in, and revert the basic example.

Otherwise, it looks great and soon ready for merge.

No it's not needed, I created a separate PR about this feature after cause of this. I will revert this commit. I will add a "no-lb" example