Skip to content
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.

Sign up for GitHub

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

Add support for upgrade policy configuration #245

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Add support for upgrade policy configuration #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ Available targets:
| <a name="input_subnet_ids"></a> [subnet\_ids](#input\_subnet\_ids) | A list of subnet IDs to launch the cluster in | `list(string)` | n/a | yes |
| <a name="input_tags"></a> [tags](#input\_tags) | Additional tags (e.g. `{'BusinessUnit': 'XYZ'}`).<br/>Neither the tag keys nor the tag values will be modified by this module. | `map(string)` | `{}` | no |
| <a name="input_tenant"></a> [tenant](#input\_tenant) | ID element \_(Rarely used, not included by default)\_. A customer identifier, indicating who this instance of a resource is for | `string` | `null` | no |
| <a name="input_upgrade_policy"></a> [upgrade\_policy](#input\_upgrade\_policy) | Configuration block for the support policy to use for the cluster | <pre>object({<br/> support_type = optional(string, null)<br/> })</pre> | `null` | no |
goruha marked this conversation as resolved.
Show resolved Hide resolved
| <a name="input_zonal_shift_config"></a> [zonal\_shift\_config](#input\_zonal\_shift\_config) | Configuration block with zonal shift configuration for the cluster | <pre>object({<br/> enabled = optional(bool, null)<br/> })</pre> | `null` | no |

## Outputs
Expand Down
1 change: 1 addition & 0 deletions docs/terraform.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
| <a name="input_subnet_ids"></a> [subnet\_ids](#input\_subnet\_ids) | A list of subnet IDs to launch the cluster in | `list(string)` | n/a | yes |
| <a name="input_tags"></a> [tags](#input\_tags) | Additional tags (e.g. `{'BusinessUnit': 'XYZ'}`).<br/>Neither the tag keys nor the tag values will be modified by this module. | `map(string)` | `{}` | no |
| <a name="input_tenant"></a> [tenant](#input\_tenant) | ID element \_(Rarely used, not included by default)\_. A customer identifier, indicating who this instance of a resource is for | `string` | `null` | no |
| <a name="input_upgrade_policy"></a> [upgrade\_policy](#input\_upgrade\_policy) | Configuration block for the support policy to use for the cluster | <pre>object({<br/> support_type = optional(string, null)<br/> })</pre> | `null` | no |
goruha marked this conversation as resolved.
Show resolved Hide resolved
| <a name="input_zonal_shift_config"></a> [zonal\_shift\_config](#input\_zonal\_shift\_config) | Configuration block with zonal shift configuration for the cluster | <pre>object({<br/> enabled = optional(bool, null)<br/> })</pre> | `null` | no |
goruha marked this conversation as resolved.
Show resolved Hide resolved

## Outputs
Expand Down
4 changes: 4 additions & 0 deletions examples/complete/fixtures.us-east-2.tfvars
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ addons = [
},
]

upgrade_policy = {
support_type = "STANDARD"
}
goruha marked this conversation as resolved.
Show resolved Hide resolved

zonal_shift_config = {
enabled = true
}
1 change: 1 addition & 0 deletions examples/complete/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ module "eks_cluster" {
addons = local.addons
addons_depends_on = [module.eks_node_group]
bootstrap_self_managed_addons_enabled = var.bootstrap_self_managed_addons_enabled
upgrade_policy = var.upgrade_policy
zonal_shift_config = var.zonal_shift_config

access_entry_map = local.access_entry_map
Expand Down
8 changes: 8 additions & 0 deletions examples/complete/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ variable "bootstrap_self_managed_addons_enabled" {
default = null
}

variable "upgrade_policy" {
type = object({
support_type = optional(string, null)
})
description = "Configuration block for the support policy to use for the cluster"
default = null
}
Comment on lines +118 to +124
Copy link

@coderabbitai coderabbitai bot Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Enhance the variable description for better clarity.

The variable implementation looks good and aligns with the PR objectives. Consider enhancing the description to include:

  • Available values for support_type (e.g., "Standard", "Extended")
  • Link to AWS documentation
  • Impact of each support type on cluster management
 variable "upgrade_policy" {
   type = object({
     support_type = optional(string, null)
   })
-  description = "Configuration block for the support policy to use for the cluster"
+  description = "Configuration block for the support policy to use for the cluster. The support_type can be set to 'Standard' or 'Extended' (default). For more information, see https://docs.aws.amazon.com/eks/latest/userguide/service-quotas.html#service-quotas-eks-support"
   default     = null
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
variable "upgrade_policy" {
type = object({
support_type = optional(string, null)
})
description = "Configuration block for the support policy to use for the cluster"
default = null
}
variable "upgrade_policy" {
type = object({
support_type = optional(string, null)
})
description = "Configuration block for the support policy to use for the cluster. The support_type can be set to 'Standard' or 'Extended' (default). For more information, see https://docs.aws.amazon.com/eks/latest/userguide/service-quotas.html#service-quotas-eks-support"
default = null
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please enhance the description. This seems like a worthy recommendation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osterman, thank you for considering the recommendation to enhance the description.


variable "zonal_shift_config" {
type = object({
enabled = optional(bool, null)
Expand Down
7 changes: 7 additions & 0 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ resource "aws_eks_cluster" "default" {
}
}

dynamic "upgrade_policy" {
for_each = var.upgrade_policy != null ? [var.upgrade_policy] : []
content {
support_type = upgrade_policy.value.support_type
}
}
goruha marked this conversation as resolved.
Show resolved Hide resolved

dynamic "zonal_shift_config" {
for_each = var.zonal_shift_config != null ? [var.zonal_shift_config] : []
content {
Expand Down
8 changes: 8 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,14 @@ variable "bootstrap_self_managed_addons_enabled" {
default = null
}

variable "upgrade_policy" {
type = object({
support_type = optional(string, null)
})
description = "Configuration block for the support policy to use for the cluster"
default = null
}
Comment on lines +206 to +212
Copy link

@coderabbitai coderabbitai bot Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Enhance the variable description with valid values and purpose.

The variable implementation is correct and aligns with AWS EKS upgrade policy configuration. However, the description could be more informative.

Consider updating the description to:

-  description = "Configuration block for the support policy to use for the cluster"
+  description = <<-EOT
+    Configuration block for the EKS cluster support policy. The support_type field accepts:
+    - "STANDARD": Shorter support timeline with lower cost
+    - "EXTENDED": Extended support with higher cost (default if not specified)
+    For more information, see https://docs.aws.amazon.com/eks/latest/userguide/eks-support-policy.html
+    EOT
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
variable "upgrade_policy" {
type = object({
support_type = optional(string, null)
})
description = "Configuration block for the support policy to use for the cluster"
default = null
}
variable "upgrade_policy" {
type = object({
support_type = optional(string, null)
})
description = <<-EOT
Configuration block for the EKS cluster support policy. The support_type field accepts:
- "STANDARD": Shorter support timeline with lower cost
- "EXTENDED": Extended support with higher cost (default if not specified)
For more information, see https://docs.aws.amazon.com/eks/latest/userguide/eks-support-policy.html
EOT
default = null
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great recommendation. We should strive for more helpful descriptions, rather than redirect the user every time to the provider documentation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osterman, thank you for your feedback. I'll ensure to provide more helpful descriptions in future recommendations.

✏️ Learnings added 
Learnt from: osterman
PR: cloudposse/terraform-aws-eks-cluster#245
File: variables.tf:206-212
Timestamp: 2024-11-27T17:49:16.591Z
Learning: When writing variable descriptions in Terraform modules, we should include valid values and detailed explanations within the code, rather than redirecting users to the provider documentation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


variable "zonal_shift_config" {
type = object({
enabled = optional(bool, null)
Expand Down