Add support for upgrade policy configuration #245
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new input parameter, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
README.md(1 hunks)docs/terraform.md(1 hunks)examples/complete/fixtures.us-east-2.tfvars(1 hunks)examples/complete/variables.tf(1 hunks)main.tf(1 hunks)variables.tf(1 hunks)
🔇 Additional comments (5)
examples/complete/fixtures.us-east-2.tfvars (1)
53-55: 🧹 Nitpick (assertive)
LGTM! Consider adding a comment about support_type options.
The upgrade_policy configuration correctly demonstrates how to set the support type to "STANDARD". To improve documentation, consider adding a comment explaining the available options (STANDARD/EXTENDED) and their implications.
Add this comment above the block:
+# Configure the EKS support policy type
+# Valid values: "STANDARD" (faster upgrades) or "EXTENDED" (longer support)
upgrade_policy = {
support_type = "STANDARD"
}Let's verify the valid values for support_type:
examples/complete/variables.tf (1)
Line range hint 112-117: Verify the impact of changing the default value to null.
The default value for bootstrap_self_managed_addons_enabled has been changed to null. This change wasn't mentioned in the PR objectives.
✅ Verification successful
Let me generate additional verification scripts to check the historical changes and usage patterns.
The change to null default value appears to be safe and intentional
Based on the verification results:
- The variable is used directly in the main module configuration as
bootstrap_self_managed_addons = var.bootstrap_self_managed_addons_enabled - The documentation in README.md and terraform.md already shows the default value as
null - The variable is only referenced in the module's core files and complete example, with consistent usage
- Setting the default to
nullallows users to explicitly opt-in to the bootstrap behavior, which is a safer default
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage and impact of bootstrap_self_managed_addons_enabled
# Check if this variable is referenced in other files
echo "Checking variable references:"
rg "bootstrap_self_managed_addons_enabled" -A 5
# Check if there are any default values in other example configurations
echo "Checking other example configurations:"
fd "variables.tf" --exec grep -l "bootstrap_self_managed_addons_enabled" {} \;
Length of output: 5041
Script:
#!/bin/bash
# Check git history for this variable to understand the change
git log -L :bootstrap_self_managed_addons_enabled:variables.tf
# Check the actual usage in main.tf to understand the impact
ast-grep --pattern 'bootstrap_self_managed_addons = $_'
# Check if there are any conditional blocks or locals using this variable
rg "bootstrap_self_managed_addons_enabled" -B 5 "locals.tf" "main.tf"
Length of output: 581
main.tf (1)
113-118: LGTM! The upgrade policy implementation looks correct.
The dynamic block implementation follows Terraform best practices:
- Conditional creation based on the presence of
var.upgrade_policy - Proper variable reference and attribute mapping
Let's verify the upgrade policy configuration:
✅ Verification successful
The upgrade policy implementation is correctly configured and used
The verification confirms that:
- The
upgrade_policyvariable is properly defined with the correct type and structure in both the root module and examples - The variable is being used correctly in the example configuration with
support_type = "STANDARD" - The dynamic block implementation in
main.tfcorrectly maps the configuration
The implementation aligns with AWS EKS upgrade policy requirements and Terraform best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the upgrade policy configuration and its usage
# Check if the upgrade_policy variable is properly defined
rg -A 5 'variable "upgrade_policy"'
# Check for any existing usage or examples of upgrade_policy
rg -A 5 'upgrade_policy.*=.*"STANDARD"'
Length of output: 742
Script:
#!/bin/bash
# Let's check for any examples or test configurations that demonstrate the usage of upgrade_policy
# Check terraform.tfvars files and example configurations
fd "terraform.tfvars|.*.tfvars" --exec cat {}
# Check test files for upgrade policy configurations
fd "test.*\.tf" --exec cat {}
# Check example configurations
fd "main.tf" --exec grep -A 10 -B 2 "upgrade_policy" {}
Length of output: 3081
variables.tf (1)
206-206: Verify the unrelated change to bootstrap_self_managed_addons_enabled.
The default value for bootstrap_self_managed_addons_enabled was changed to null. This change appears unrelated to the PR's objective of adding upgrade policy support.
README.md (1)
457-457: LGTM! Documentation for the new input parameter is clear.
The upgrade_policy input parameter is well-documented in the inputs section.
| variable "upgrade_policy" { | ||
| type = object({ | ||
| support_type = optional(string, null) | ||
| }) | ||
| description = "Configuration block for the support policy to use for the cluster" | ||
| default = null | ||
| } |
There was a problem hiding this comment.
🧹 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.
| 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 | |
| } |
There was a problem hiding this comment.
Please enhance the description. This seems like a worthy recommendation.
There was a problem hiding this comment.
@osterman, thank you for considering the recommendation to enhance the description.
| variable "upgrade_policy" { | ||
| type = object({ | ||
| support_type = optional(string, null) | ||
| }) | ||
| description = "Configuration block for the support policy to use for the cluster" | ||
| default = null | ||
| } |
There was a problem hiding this comment.
🧹 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.
| 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 | |
| } |
There was a problem hiding this comment.
This is a great recommendation. We should strive for more helpful descriptions, rather than redirect the user every time to the provider documentation.
There was a problem hiding this comment.
@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.
|
FYI I'm ignoring any AI recommendation. |
|
@z0rc, thanks for your contribution. |
|
/terratest |
@goruha It should be used. I added it with intention to test the change. Value is passed here This change is done in the same style as previous PR #243. Let me double check if anything is missing. |
|
@goruha ready to test. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
README.md(1 hunks)docs/terraform.md(1 hunks)examples/complete/fixtures.us-east-2.tfvars(1 hunks)examples/complete/main.tf(1 hunks)examples/complete/variables.tf(1 hunks)main.tf(1 hunks)variables.tf(1 hunks)
🔇 Additional comments (8)
examples/complete/fixtures.us-east-2.tfvars (1)
53-55: Verify upgrade policy configuration against AWS specifications.
Since this is a test fixture, we should ensure it properly tests the upgrade policy configuration according to AWS specifications.
Let's verify the configuration against the codebase and documentation:
✅ Verification successful
Let me gather more specific information about the upgrade policy implementation.
The upgrade policy configuration is correctly implemented
The verification shows that:
- The variable is properly defined with an optional
support_typestring field - The implementation in
main.tfcorrectly applies the configuration using a dynamic block - The test fixture in
fixtures.us-east-2.tfvarsprovides a valid test case withSTANDARDsupport type
The configuration aligns with AWS specifications for Amazon OpenSearch Service upgrade policies, where STANDARD is a valid support type value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify upgrade policy configuration implementation and documentation
# Check if the variable is properly defined in variables.tf
echo "Checking variable definition..."
rg -A 5 "variable.*upgrade_policy"
# Check if there are any validation blocks for the upgrade_policy
echo "Checking validation rules..."
rg "validation.*upgrade_policy"
# Check if the README documents the valid values
echo "Checking documentation..."
rg -A 3 "upgrade_policy" README.md
Length of output: 1473
Script:
#!/bin/bash
# Search for upgrade_policy usage in main.tf files
echo "Checking upgrade_policy implementation..."
rg -A 5 "upgrade_policy" main.tf
# Search for any test cases involving upgrade_policy
echo "Checking test cases..."
rg -A 5 "upgrade_policy.*support_type" test/
# Check other tfvars files for upgrade_policy examples
echo "Checking other tfvars files..."
fd -e tfvars -x rg -l "upgrade_policy"
Length of output: 673
examples/complete/variables.tf (1)
118-124: 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
}examples/complete/main.tf (1)
116-116: LGTM! The upgrade_policy parameter is properly integrated.
The addition of upgrade_policy is well-aligned with the module's configuration style and matches the PR objectives.
Let's verify the variable definition and documentation:
✅ Verification successful
Variable definition and documentation are properly in place
The verification confirms:
- Variable
upgrade_policyis properly defined in both root and examplevariables.tffiles with correct type definition and description - Documentation is present in both
README.mdanddocs/terraform.mdwith complete variable details including type, description, and default value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for variable definition and documentation
# Check for variable definition in variables.tf
echo "Checking for variable definition..."
rg -A 5 'variable.*"upgrade_policy"' --type tf
# Check for documentation
echo "Checking for documentation..."
rg -A 5 'upgrade_policy' --type md
Length of output: 2123
main.tf (1)
113-118: LGTM! Clean implementation of the upgrade policy configuration.
The dynamic block follows consistent patterns with other configurations in the file and correctly implements the EKS cluster upgrade policy support.
variables.tf (2)
206-212: LGTM on the variable structure!
The implementation of upgrade_policy variable is well-structured:
- Correctly uses object type with optional field
- Properly sets default to null making it optional
- Matches AWS provider expectations
210-211: Enhance the variable description with valid values and purpose.
The description should be more informative to help users understand the available options and their implications.
- 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
+ EOTdocs/terraform.md (1)
112-112: Enhance the documentation for the upgrade_policy variable.
The description needs to be more detailed to help users understand the purpose and impact of this setting.
README.md (1)
457-457: 🧹 Nitpick (assertive)
Enhance documentation for the upgrade_policy parameter.
The documentation for the upgrade_policy parameter needs to be enhanced with:
- Description explaining that it configures the EKS support policy
- Valid values for
support_type("Standard", "Extended") - Link to AWS documentation
- Example usage
Apply this diff to improve the documentation:
-| <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 |
+| <a name="input_upgrade_policy"></a> [upgrade\_policy](#input\_upgrade\_policy) | Configuration block for the EKS support policy to use for the cluster. The support policy determines the duration for which a Kubernetes version is supported after it is deprecated. For more information, see [Amazon EKS Kubernetes versions](https://docs.aws.amazon.com/eks/latest/userguide/kubernetes-versions.html). | <pre>object({<br/> support_type = optional(string, null) # Valid values: "Standard", "Extended"<br/> })</pre> | `null` | no |
Example usage:
```hcl
module "eks_cluster" {
source = "cloudposse/eks-cluster/aws"
# ... other configuration ...
upgrade_policy = {
support_type = "Standard" # Change from default "Extended" support policy
}
}
|
/terratest |
|
These changes were released in v4.5.0. |
@z0rc please don't as these were mostly great suggestions, better than our own reviews =) |
what
Add module variable to set
aws_eks_cluster'supgrade_policy.why
I need to be able to downgrade EKS support policy from default "Extedned" to "Standard".
references
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_cluster#upgrade_policy
https://aws.amazon.com/about-aws/whats-new/2024/07/amazon-eks-controls-kubernetes-version-support-policy/
Summary by CodeRabbit
New Features
upgrade_policyfor configuring the support policy of the EKS cluster.zonal_shift_configto the Terraform configuration.Documentation
upgrade_policyinput.upgrade_policyconfiguration in the fixtures and variable files.Bug Fixes