-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
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 <33028589+dpiddockcmp@users.noreply.github.com>
Co-authored-by: Daniel Piddock <33028589+dpiddockcmp@users.noreply.github.com>
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
policy_arn = "${var.iam_policy_arn_prefix}/AmazonEKSFargatePodExecutionRolePolicy" | ||
role = aws_iam_role.eks_fargate_pod[0].name | ||
} | ||
|
||
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