-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: Add EKS Fargate support #1067
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
feat: Add EKS Fargate support #1067
Conversation
|
@itssimon I updated your PR and fixed suggested reviews. I've also add example. Can you review this please. Community: We need help to test this. I would like to merge this during the week. |
|
@sc250024 sounds like you're also working on fargate support. Can you review this please ? |
Co-authored-by: Daniel Piddock <[email protected]>
Co-authored-by: Daniel Piddock <[email protected]>
antonbabenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good.
| @@ -0,0 +1,10 @@ | |||
| locals { | |||
| create_eks = var.create_eks && length(var.fargate_profiles) > 0 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to introduce another variable that should be known in advance (eg, type boolean) to be able to use it in count. Something like create_eks_fargate = true.
The way it is written now fargate_profiles can't contain references to not-yet-created resources and modules inside of the value.
modules/fargate/fargate.tf
Outdated
|
|
||
| resource "aws_eks_fargate_profile" "this" { | ||
| for_each = local.create_eks ? local.fargate_profiles_expanded : {} | ||
| for_each = var.create_eks ? local.fargate_profiles_expanded : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This for_each will fail if local.fargate_profiles_expanded contains not-yet-known values (as you've just replaced for local.create_eks). I am not sure what is the best solution for this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var.fargate_profiles and local.fargate_profiles_expanded are maps and to work with for_each, their keys must be known during plan. If keys are known, I think the Terraform will wait correctly for aws_eks_fargate_profile creation if there is a not yet known values inside local.fargate_profiles_expanded.
So length(var.fargate_profiles) will be known because keys must be known.
|
Thanks again @itssimon for all your work on this feature. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
PR o'clock
Description
This PR is taken from #866 and add suggested review.
Checklist