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.

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

Fix EFS volume configuration getting discarded #235

Closed
wants to merge 2 commits into from

Conversation

vanilla-lake
Copy link

@vanilla-lake vanilla-lake commented Jul 13, 2023

what

  • Docker and EFS volume configuration objects are now getting passed to either the ecs-alb-service-task module's docker_volumes or efs_volumes variables based on whether they have non-empty docker_volume_configuration or efs_volume_configuration lists.

why

  • These changes fix the EFS configuration object getting discarded due to the volumes variable only getting passed to the ecs-alb-service-task module's docker_volumes variable.

references

@vanilla-lake vanilla-lake requested review from a team as code owners July 13, 2023 14:10
@@ -167,7 +167,8 @@ module "ecs_alb_service_task" {
subnet_ids = var.ecs_private_subnet_ids
container_port = var.container_port
nlb_container_port = var.nlb_container_port
docker_volumes = var.volumes
docker_volumes = [for volume in var.volumes : volume if length(volume.docker_volume_configuration) > 0]
Copy link
Member

Choose a reason for hiding this comment

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

These conditions might be better in the ecs alb service task module instead of here

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task

Copy link
Contributor

Choose a reason for hiding this comment

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

@nitrocode Wouldn't that require the ecs alb service task module to be refactored to accept a generic volumes variable?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm i don't imagine there would be much of a refactoring. I'll defer to the @cloudposse/contributors if they have an opinion on it

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to move this to ecs alb service task if needed. Just let me know what needs to be done to get this fix merged please

@hans-d hans-d added the stale This PR has gone stale label Mar 2, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@mergify mergify bot added triage Needs triage and removed stale This PR has gone stale labels Apr 1, 2024
@kevcube
Copy link
Contributor

kevcube commented Aug 19, 2024

replaced by #284

@kevcube kevcube closed this Aug 19, 2024
@mergify mergify bot removed the triage Needs triage label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to create EFS volume EFS Volume configuration does not get passed on
7 participants