feat: lookup for AMI that doesn't contain ECS/EKS agent and packages + enable setting volume size #30
Conversation
WalkthroughThis pull request encompasses a comprehensive update to the repository's configuration files, focusing on enhancing development workflows, linting rules, and infrastructure management. The changes span multiple configuration files, including GitHub Actions, Trunk, and Terraform settings. Key modifications include updating workflow action versions, refining Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Trunk as Trunk Configuration
participant GitHub as GitHub Actions
participant AWS as AWS Resources
Dev->>Trunk: Update configuration files
Trunk->>GitHub: Trigger workflow with new action version
GitHub->>AWS: Deploy infrastructure
AWS-->>GitHub: Validate instance type and AMI
Possibly Related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
data.tf (1)
11-11: Consider adding explicit filters to exclude ECS/EKS AMIsWhile the current pattern helps select AL2023 AMIs, consider adding an explicit filter to exclude ECS/EKS AMIs for better clarity and maintainability:
filter { name = "name" values = ["al2023-ami-2023*"] } +filter { + name = "name" + values = ["al2023-ami-2023*-*"] +} + +filter { + name = "name" + values = ["^((?!ecs).)*$", "^((?!eks).)*$"] +}main.tf (1)
334-337: Consider adding volume type configurationWhile the volume configuration includes size and encryption, consider adding volume type configuration for better control over performance and cost:
block_device_mappings { device_name = "/dev/xvda" ebs { encrypted = true volume_size = var.volume_size + volume_type = var.volume_type + iops = var.volume_type == "io1" || var.volume_type == "io2" ? var.iops : null + throughput = var.volume_type == "gp3" ? var.throughput : null } }Add corresponding variables to
variables.tf:variable "volume_type" { description = "Type of volume. Can be 'gp2', 'gp3', 'io1', 'io2', 'sc1', or 'st1'." type = string default = "gp3" } variable "iops" { description = "IOPS for the EBS volume (required for io1/io2)." type = number default = null } variable "throughput" { description = "Throughput (MiBps) for gp3 volumes." type = number default = null }README.md (1)
171-171: Enhance volume_size parameter description.Consider adding more context to the volume_size parameter description, such as the default value if null, minimum/maximum allowed values, and any constraints.
-| <a name="input_volume_size"></a> [volume_size](#input_volume_size) | The size of the volume in gigabytes. | `number` | `null` | no | +| <a name="input_volume_size"></a> [volume_size](#input_volume_size) | The size of the root volume in gigabytes. If null, the default size from the AMI will be used. Minimum value is 8 GB for Amazon Linux 2023. | `number` | `null` | no |
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/trunk-upgrade.yaml(1 hunks).trunk/.gitignore(1 hunks).trunk/configs/.checkov.yaml(1 hunks).trunk/configs/.markdownlint.yaml(1 hunks).trunk/configs/.prettierignore(1 hunks).trunk/configs/.yamllint.yaml(1 hunks).trunk/trunk.yaml(1 hunks)README.md(2 hunks)data.tf(1 hunks)main.tf(1 hunks)variables.tf(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- .trunk/configs/.prettierignore
- .trunk/configs/.checkov.yaml
- .trunk/.gitignore
- .github/trunk-upgrade.yaml
- .trunk/configs/.markdownlint.yaml
- .trunk/trunk.yaml
🧰 Additional context used
📓 Path-based instructions (3)
data.tf (1)
Pattern **/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
variables.tf (1)
Pattern **/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
main.tf (1)
Pattern **/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
🔇 Additional comments (8)
.trunk/configs/.yamllint.yaml (4)
2-4: LGTM! Well-configured string quoting rules.The
quoted-stringsconfiguration strikes a good balance between readability and functionality by:
- Only requiring quotes when needed
- Supporting template expressions with
{|}characters
8-8: LGTM! Essential duplicate key prevention.Preventing duplicate keys is crucial for maintaining YAML validity and avoiding ambiguity.
9-10: LGTM! Good practice for octal values.Forbidding implicit octal values prevents confusion that could arise from numbers with leading zeros.
5-7: Consider if strict empty value prohibition is necessary.The current configuration forbids empty values in both block and flow mappings, which might be too restrictive for valid use cases like optional fields or placeholder configurations.
Consider relaxing this rule if your project needs to support empty values in certain scenarios:
empty-values: - forbid-in-block-mappings: true - forbid-in-flow-mappings: true + forbid-in-block-mappings: false + forbid-in-flow-mappings: false✅ Verification successful
Let me search for YAML files in the codebase to understand the context better.
Let me check the actual content of the YAML files to better understand how empty values are being used.
Keep the strict empty value prohibition in YAML configurations
Based on the analysis of the repository's YAML files, I recommend keeping the strict empty value prohibition. The codebase follows a clear and explicit configuration style where:
- Configuration keys either have specific values or are structured with nested mappings
- Empty values are properly handled using explicit YAML syntax (e.g., arrays, nested structures)
- No instances of intentionally empty values were found that would benefit from relaxing these rules
The current strict configuration helps maintain clarity and prevents ambiguous configurations, which is particularly important for infrastructure and tooling configurations present in this repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find all YAML files in the repository fd -e yaml -e yml # Check for any existing empty values in YAML files rg ":\s*$" -t yamlLength of output: 1212
Script:
#!/bin/bash # Check content of the YAML files cat aqua.yaml .coderabbit.yaml .pre-commit-config.yaml # Specifically look for any patterns where empty values might be intentional rg ":\s*(null|~|''|\"\")(\s*#.*)?$" -t yamlLength of output: 3532
main.tf (1)
Line range hint
1-21: Well-implemented instance type validation!Excellent implementation of architecture compatibility checks with clear error messages. The validation ensures that ARM instances are used with arm64 architecture and x86 instances with x86_64 architecture.
README.md (3)
Line range hint
10-17: LGTM! Well-documented dependencies.The dependencies section is clear, well-structured, and includes proper links to all required modules.
176-184: LGTM! Clear and well-documented outputs.The outputs section provides clear descriptions for all exported values.
76-88: Verify provider version compatibility.The requirements and providers sections are well-documented. However, let's verify the compatibility of the specified provider versions with the module's functionality.
✅ Verification successful
Provider versions are correctly configured and compatible
The provider version constraints in
versions.tfperfectly match the requirements documented in the README:
- AWS provider: >= 5.0
- Null provider: >= 3.2
- Time provider: >= 0.7
The specific provider versions listed in the README ([email protected], [email protected]) are within these constraints, ensuring compatibility with the module's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any provider version constraints in Terraform files # and compare with the documented versions. # Search for provider version constraints in .tf files echo "Checking provider version constraints in Terraform files..." rg -A 2 'required_providers|provider\s+"aws"|provider\s+"null"' '*.tf'Length of output: 246
Script:
#!/bin/bash # Let's try searching in the entire repository for Terraform files echo "Looking for Terraform files..." fd -e tf echo -e "\nChecking provider configurations..." rg -U "terraform\s+{[^}]*required_providers[^}]*}|provider\s+\"(?:aws|null)\"\s+{[^}]*}" --type tfLength of output: 590
Script:
#!/bin/bash echo "Checking versions.tf content..." cat versions.tf echo -e "\nChecking provider configurations in main.tf..." cat main.tf | grep -A 5 "provider\|required_providers"Length of output: 558
| variable "volume_size" { | ||
| description = "The size of the volume in gigabytes." | ||
| type = number | ||
| default = null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and improve documentation for volume_size variable
Consider adding validation for the volume size and enhancing the documentation with AWS constraints:
variable "volume_size" {
- description = "The size of the volume in gigabytes."
+ description = <<-EOT
+ The size of the root volume in gigabytes. If not specified, defaults to the AMI's root volume size.
+ Constraints:
+ - gp2/gp3: 1-16,384 GiB
+ - io1/io2: 4-16,384 GiB
+ - st1: 125-16,384 GiB
+ - sc1: 125-16,384 GiB
+ EOT
type = number
default = null
+
+ validation {
+ condition = var.volume_size == null || (var.volume_size >= 1 && var.volume_size <= 16384)
+ error_message = "Volume size must be between 1 and 16,384 GiB for gp2/gp3 volumes."
+ }
}📝 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 "volume_size" { | |
| description = "The size of the volume in gigabytes." | |
| type = number | |
| default = null | |
| } | |
| variable "volume_size" { | |
| description = <<-EOT | |
| The size of the root volume in gigabytes. If not specified, defaults to the AMI's root volume size. | |
| Constraints: | |
| - gp2/gp3: 1-16,384 GiB | |
| - io1/io2: 4-16,384 GiB | |
| - st1: 125-16,384 GiB | |
| - sc1: 125-16,384 GiB | |
| EOT | |
| type = number | |
| default = null | |
| validation { | |
| condition = var.volume_size == null || (var.volume_size >= 1 && var.volume_size <= 16384) | |
| error_message = "Volume size must be between 1 and 16,384 GiB for gp2/gp3 volumes." | |
| } | |
| } |
what
why
al2023-ami-ecs-hvm-2023.0.20241217-kernel-6.1-x86_64(Amazon Linux AMI 2023.0.20241217 x86_64 ECS HVM EBS). It's pre-configured for ECS, including the Amazon ECS agent and container runtime. We don't need those packages.references
Summary by CodeRabbit
Release Notes
New Features
Configuration Updates
Documentation
Infrastructure