search

fpco / terraform-aws-foundation

Establish a solid Foundation on AWS with these modules for Terraform
MIT License
204 stars 98 forks source link

Initial support for Terraform v0.12.x #219

Closed ketzacoatl closed 5 years ago

ketzacoatl commented 5 years ago

Thanks for @Magicloud for tackling the repo-wide upgrade! The MR has been testing well, but it's likely imperfect. I'd rather release and fix and release updates, than hold this back.

Magicloud commented 5 years ago

One thing I am not sure is the hardcoded version of terraform in Makefile. Why 0.12.2 when latest release is 0.12.4? I think this also affects default-build-image. If we want to stick to a non-latest version, I'd suggest to have a KV DB to store those kind of "company preference". So every where, it is the right version.

Magicloud commented 5 years ago

The review system is bit of weird, so I post some comments without a link to source.

In examples/load-asg/main.tf, around line 105, you removed the TODO generated by TF. That is fine. But seems like you also removed two other comments, started with "Ensure", and "ELBs". Just to confirm it is on purpose.

Magicloud commented 5 years ago

And there is syntax error, for example, examples/kube-stack-public/kube.tf, line 24.

It is a lint for one element list. But the closing bracket is gone.

ketzacoatl commented 5 years ago

Is the code review physically not working? It would be helpful if you could put the comments at the relevant code in the future. Thank you for the review, and you are correct, removing the TODO should not have removed the next line.

Magicloud commented 5 years ago

Sorry, get it working now. Posted my comments.

And for syntax error, I think I made mistake. I said multiple, but reviewing today gets only one.

ketzacoatl commented 5 years ago

@qrilka / @Magicloud ok, I have updated the branch again. Please review one last time, give approval if that is ready, and we can merge.