feat: Add ability to define custom timeout for fargate profiles by IvanDechovsky · Pull Request #1614 · terraform-aws-modules/terraform-aws-eks

IvanDechovsky · 2021-09-30T21:50:20Z

Add ability to define custom timeout for create/delete operations for fargate profiles

daroga0002

This change seems be not tested, please test and fix a comments

Additionally I don`t know does changes in README.md where done by you or any automatic ?? (we use precommit hooks to generate some part of docs using terraform-docs)

daroga0002 · 2021-10-01T10:04:47Z

      ]

+      timeouts = {
+        create = "15"


Error: Error parsing "create" timeout: time: missing unit in duration "15"

daroga0002 · 2021-10-01T10:04:56Z


+      # Use custom defaults for create/delete operations
+      timeouts = {
+        create = "15"


Error: Error parsing "create" timeout: time: missing unit in duration "15"

daroga0002 · 2021-10-01T10:05:15Z


+      # Use custom defaults for create/delete operations
+      timeouts = {
+        create = "15"


Error: Error parsing "create" timeout: time: missing unit in duration "15"

daroga0002 · 2021-10-01T10:05:45Z

-| subnets | List of subnet IDs. Will replace the root module subnets. | `list(string)` | `var.subnets` | no |
-| tags | Key-value map of resource tags. Will be merged with root module tags. | `map(string)` | `var.tags` | no |
+| Name      | Description                                                                      | Type                                                                       | Default                                                                                   | Required |
+| --------- | -------------------------------------------------------------------------------- | -------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------- | :------: |


does those changes where done by you or by some automation?

daroga0002 · 2021-10-01T10:06:20Z

-| <a name="output_fargate_profile_ids"></a> [fargate\_profile\_ids](#output\_fargate\_profile\_ids) | EKS Cluster name and EKS Fargate Profile names separated by a colon (:). |
-| <a name="output_iam_role_arn"></a> [iam\_role\_arn](#output\_iam\_role\_arn) | IAM role ARN for EKS Fargate pods |
-| <a name="output_iam_role_name"></a> [iam\_role\_name](#output\_iam\_role\_name) | IAM role name for EKS Fargate pods |
+| Name                                                                                                 | Description                                                              |


what has done those changes?

this part of file should be autogenerated via pre commit hooks

daroga0002 · 2021-10-01T10:07:53Z

  }

+  timeouts {
+    create = lookup(each.value["timeouts"], "create", 10)


instead assuming some values put failover to null value which will use provider defaults

Suggested change

create = lookup(each.value["timeouts"], "create", 10)

create = lookup(each.value["timeouts"], "create", null)

daroga0002 · 2021-10-01T10:08:15Z


+  timeouts {
+    create = lookup(each.value["timeouts"], "create", 10)
+    delete = lookup(each.value["timeouts"], "delete", 10)


Suggested change

delete = lookup(each.value["timeouts"], "delete", 10)

delete = lookup(each.value["timeouts"], "delete", null)

daroga0002 · 2021-10-02T07:58:21Z

  default     = {}
 }

+variable "fargate_profiles_create_timeout" {


We dont want add additionall variables into submodule, it should follow previously used pattern. I dont know why you changed this as I given you direct suggestion how to make default null value

daroga0002

We should same pattern across timeouts (was used previously in this PR, I dont know why you changed)

IvanDechovsky · 2021-10-08T10:24:08Z

@daroga0002 Do you mind taking a look again?

daroga0002 · 2021-10-08T10:31:01Z

does this was tested?

as I get on plan phase:

╷
│ Error: Invalid index
│ 
│   on ../../modules/fargate/main.tf line 67, in resource "aws_eks_fargate_profile" "this":
│   67:     create = lookup(each.value["timeouts"], "create", null)
│     ├────────────────
│     │ each.value is object with 3 attributes
│ 
│ The given key does not identify an element in this collection value.
╵
╷
│ Error: Invalid index
│ 
│   on ../../modules/fargate/main.tf line 68, in resource "aws_eks_fargate_profile" "this":
│   68:     delete = lookup(each.value["timeouts"], "delete", null)
│     ├────────────────
│     │ each.value is object with 3 attributes
│ 
│ The given key does not identify an element in this collection value.

IvanDechovsky · 2021-10-11T12:10:44Z

@daroga0002 Updated. The problem was that if timeouts is not defined the lookup() function throws the error you saw, instead of assigning the default value (null). I updated the logic and verified it works in both scenarios. Please review again.

IvanDechovsky · 2021-10-18T08:51:08Z

@daroga0002 Will you have some time to review it this week perhaps?

mhulscher · 2021-10-20T07:25:47Z

Any chance this can make it in the next release? For some reason fargate profiles take considerably longer to create nowadays. Most of our terraform applies are failing because of this timeout, and it is failing our testing pipelines :(

IvanDechovsky · 2021-11-02T10:47:19Z

Any chance this can be looked at please?

daroga0002 · 2021-11-02T11:01:32Z

I have this on my list, will do this till end of week

daroga0002

Please add into file
https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/modules/fargate/README.md

in line 17 following content:

| timeouts | A map of timeouts for create/delete operations. | `map(string)` | Provider default behavior | no |

daroga0002 · 2021-11-02T12:09:48Z

+
+      # Set custom timeout for create/delete operation on fargate profiles
+      timeouts = {
+        create = "20m"


lets remove create just for test case when we have just one subargument

daroga0002 · 2021-11-02T12:10:23Z

        Owner = "secondary"
      }
+
+      # Set custom timeout for create/delete operation on fargate profiles


remove lines 99-103 for test case when we dont define any timeout arguments

daroga0002 · 2021-11-02T12:12:25Z

@IvanDechovsky thank you for your contribution, 3 comments and after fixing we will be able to merge it

…ns for fargate profiles

IvanDechovsky · 2021-11-02T21:40:17Z

@daroga0002 Thank you for taking a look. Updated!

daroga0002

One typo in behaviour to fix and we can merge.

@antonbabenko this can be merged when typo will be fixed (maybe you can accept commit on suggestion?)

# [17.24.0](v17.23.0...v17.24.0) (2021-11-22) ### Bug Fixes * Added Deny for CreateLogGroup action in EKS cluster role ([#1594](#1594)) ([6959b9b](6959b9b)) * update CI/CD process to enable auto-release workflow ([#1698](#1698)) ([b876ff9](b876ff9)) ### Features * Add ability to define custom timeout for fargate profiles ([#1614](#1614)) ([b7539dc](b7539dc)) * Removed ng_depends_on variable and related hack ([#1672](#1672)) ([56e93d7](56e93d7))

antonbabenko · 2021-11-22T16:17:26Z

This PR is included in version 17.24.0 🎉

…aform-aws-modules#1614)

github-actions · 2022-11-11T02:34:31Z

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.

IvanDechovsky mentioned this pull request Sep 30, 2021

Set custom timeouts for create and delete in fargate_profiles input #1610

Closed

daroga0002 suggested changes Oct 1, 2021

View reviewed changes

IvanDechovsky force-pushed the add_timeoute_fargate_profile branch from 5b22760 to a7ee60f Compare October 1, 2021 21:20

daroga0002 reviewed Oct 2, 2021

View reviewed changes

daroga0002 suggested changes Oct 2, 2021

View reviewed changes

IvanDechovsky force-pushed the add_timeoute_fargate_profile branch 5 times, most recently from a50482f to 89bd8fb Compare October 4, 2021 13:16

IvanDechovsky force-pushed the add_timeoute_fargate_profile branch 2 times, most recently from 7cf2229 to 2da840c Compare October 11, 2021 12:08

IvanDechovsky force-pushed the add_timeoute_fargate_profile branch 3 times, most recently from 446ddb7 to 2cea749 Compare October 14, 2021 11:27

IvanDechovsky force-pushed the add_timeoute_fargate_profile branch from 2cea749 to 7ae6fc7 Compare October 18, 2021 10:47

daroga0002 approved these changes Nov 2, 2021

View reviewed changes

daroga0002 suggested changes Nov 2, 2021

View reviewed changes

feat: Add ability to define custom timeout for create/delete operatio…

6ecd9ec

…ns for fargate profiles

IvanDechovsky force-pushed the add_timeoute_fargate_profile branch from 7ae6fc7 to 6ecd9ec Compare November 2, 2021 21:39

daroga0002 reviewed Nov 3, 2021

View reviewed changes

Comment thread modules/fargate/README.md

daroga0002 approved these changes Nov 3, 2021

View reviewed changes

daroga0002 requested a review from antonbabenko November 3, 2021 09:22

Simplify code and removed comments

118e377

antonbabenko merged commit b7539dc into terraform-aws-modules:master Nov 3, 2021

astech-mweber3 pushed a commit to spring-media/terraform-aws-eks that referenced this pull request Dec 1, 2021

feat: Add ability to define custom timeout for fargate profiles (terr…

40db2de

…aform-aws-modules#1614)

github-actions Bot locked as resolved and limited conversation to collaborators Nov 11, 2022

	create = lookup(each.value["timeouts"], "create", 10)
	create = lookup(each.value["timeouts"], "create", null)

	delete = lookup(each.value["timeouts"], "delete", 10)
	delete = lookup(each.value["timeouts"], "delete", null)

Uh oh!

Conversation

IvanDechovsky commented Sep 30, 2021

Uh oh!

daroga0002 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daroga0002 left a comment

Choose a reason for hiding this comment

Uh oh!

IvanDechovsky commented Oct 8, 2021

Uh oh!

daroga0002 commented Oct 8, 2021

Uh oh!

IvanDechovsky commented Oct 11, 2021

Uh oh!

IvanDechovsky commented Oct 18, 2021

Uh oh!

mhulscher commented Oct 20, 2021

Uh oh!

IvanDechovsky commented Nov 2, 2021

Uh oh!

daroga0002 commented Nov 2, 2021

Uh oh!

daroga0002 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daroga0002 commented Nov 2, 2021

Uh oh!

IvanDechovsky commented Nov 2, 2021

Uh oh!

Uh oh!

daroga0002 left a comment

Choose a reason for hiding this comment

Uh oh!

antonbabenko commented Nov 22, 2021

Uh oh!

github-actions Bot commented Nov 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants