search

arminc / terraform-ecs

AWS ECS terraform module
MIT License
804 stars 374 forks source link

These two lines appear to have issues. #39

Closed JustinTRoss closed 3 years ago

JustinTRoss commented 3 years ago

Disclaimer: I don't fully understand the context for this file or this portion of the file, and I'm not especially versed in shell scripting.

These two lines appear to me to have issues.

https://github.com/arminc/terraform-ecs/blob/7eb8a43154e61e68d50fd749c48cb17da65aacbb/modules/ecs_instances/templates/user_data.sh#L97-L98

Should az=$(curl -s http://instance-data/latest/meta-data/placement/availability-zone) be az=$(curl -s http://169.254.169.254/latest/meta-data/placement/availability-zone)

and should region=$${az:0:$${#az} - 1} be region=$${az:0:$${az} - 1}

Would you mind briefly confirming for me and possibly explaining the context for this file and these lines?

arminc commented 3 years ago

As stated above in the comment: "Get ECS instance info, although not used in this user_data itself" the code below that is not used by the script, therefore we would just remove all the lines or fix them as that line is wrong as you correctly point out.

JustinTRoss commented 3 years ago

Thanks. I understand the lines aren't used in user_data.sh itself. Forgive my inexperience with Terraform, but where are they be useful otherwise?

Also, I realize now region=$${az:0:$${#az} - 1} could actually just be region=$${az:0:-1} to get az without the last char. Will this ever differ from the region we specify through terraform variable?

arminc commented 3 years ago

This could be useful when you pass in your own "custom_userdata" to the script and then you already have the 'region' information that you can use. But I think we should remove these lines from the script as the region is already defined on line 60.

P.S. Don't worry about the experience, we are all always learning something new :)