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 external task definition must exist before first run #204

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

mightyguava
Copy link
Contributor

what

Change external task definition to a list(string) so that it can be flagged on without needing the task definition to already exist.

why

Fix this issue when using an external task definition

│   49:   count                    = local.enabled && var.task_definition == null ? 1 : 0
│
│ The "count" value depends on resource attributes that cannot be determined
│ until apply, so Terraform cannot predict how many instances will be
│ created. To work around this, use the -target argument to first apply only
│ the resources that the count depends on.

references

Similar to how this was solved for the task role arn https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/blob/main/variables.tf#L216-L226. The workaround for now is to use -target like mentioned in #123.

@goruha
Copy link
Member

goruha commented Jun 5, 2023

Hello @mightyguava.

Thanks for your contribution.

Could you pls fix the following issues:

  1. Code style:
  main.tf:364:83: warning: List items should be accessed using square brackets ()

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/actions/runs/5179362958/jobs/9332797160?pr=204#step:6:120

  1. Update README by running
README.md is outdated. Please run the following commands locally and push the file:
  make init
  make readme

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/actions/runs/5179362958/jobs/9332793659?pr=204

Thanks.

@goruha
Copy link
Member

goruha commented Jun 5, 2023

/terratest

@mightyguava
Copy link
Contributor Author

mightyguava commented Jun 5, 2023

hey @goruha i'm happy to fix the lint issue, but wanted to double-check that it won't break any backwards compatibility contract the module has? The lint is complaining about "${join("", aws_ecs_task_definition.default.*.family)}:${join("", aws_ecs_task_definition.default.*.revision)}" which matches the original code on main.

@goruha goruha self-assigned this Jun 6, 2023
@goruha
Copy link
Member

goruha commented Jun 6, 2023

@mightyguava
I checked tflint log again.

It failed because of runs out of warning limits.
All warnings are about list items' access

aws_ecs_task_definition.default.*.family should be aws_ecs_task_definition.default[*].family

Could you fix all of them?

  • main.tf:364:40: warning: List items should be accessed using square brackets ()
  • main.tf:364:83: warning: List items should be accessed using square brackets ()
  • main.tf:364:137: warning: List items should be accessed using square brackets ()
  • main.tf:463:40: warning: List items should be accessed using square brackets ()
  • main.tf:463:83: warning: List items should be accessed using square brackets ()
  • main.tf:463:137: warning: List items should be accessed using square brackets ()
  • main.tf:562:40: warning: List items should be accessed using square brackets ()
  • main.tf:562:83: warning: List items should be accessed using square brackets ()
  • main.tf:562:137: warning: List items should be accessed using square brackets ()
  • main.tf:661:40: warning: List items should be accessed using square brackets ()
  • main.tf:661:83: warning: List items should be accessed using square brackets ()
  • main.tf:661:137: warning: List items should be accessed using square brackets ()

@mightyguava
Copy link
Contributor Author

Done!

@goruha
Copy link
Member

goruha commented Jun 6, 2023

/terratest

@goruha
Copy link
Member

goruha commented Jun 6, 2023

@mightyguava Thanks for your contribution.

LGTM

@goruha goruha merged commit 48f5647 into cloudposse:main Jun 6, 2023
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.

2 participants