-
Notifications
You must be signed in to change notification settings - Fork 31
fix: define container_insights as an enum #69
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
Conversation
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
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.
Overall, this is a good improvement to the ECS module. The changes make the container insights configuration more robust by:
- Converting container_insights to an enum with proper validation
- Adding support for the "enhanced" container insights option
- Properly handling service connect defaults in the lifecycle block
The suggestions provided would improve documentation and maintainability, but the core changes are solid and ready to be merged after addressing the feedback.
|
|
||
| variable "container_insights" { | ||
| type = bool | ||
| default = false | ||
| type = string |
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.
Good improvement to the container insights configuration. Using an enum with validation ensures that only valid values are provided and adds support for the "enhanced" option. Consider adding a description of what each value means to help users choose the appropriate setting.
| variable "container_insights" { | |
| type = bool | |
| default = false | |
| type = string | |
| variable "container_insights" { | |
| type = string | |
| default = "disabled" | |
| description = <<-EOT | |
| Enables CloudWatch Container Insights for a cluster. | |
| Valid values: | |
| - "enhanced": Enables enhanced container insights with additional metrics | |
| - "enabled": Enables basic container insights | |
| - "disabled": Disables container insights | |
| EOT | |
| validation { | |
| condition = contains(["enhanced", "enabled", "disabled"], var.container_insights) | |
| error_message = "Container Insights must be one of: enhanced, enabled, disabled." | |
| } | |
| } |
| lifecycle { | ||
| ignore_changes = [] | ||
| ignore_changes = [service_connect_defaults] | ||
|
|
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.
Good addition of service_connect_defaults to ignore_changes. This prevents unwanted updates when service connect configurations are modified outside of Terraform. Consider adding a comment explaining why this change is needed for better maintainability.
| lifecycle { | |
| ignore_changes = [] | |
| ignore_changes = [service_connect_defaults] | |
| lifecycle { | |
| # Ignore changes to service connect defaults as they may be modified outside of Terraform | |
| ignore_changes = [service_connect_defaults] | |
| } |
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.
LGTM
Define
setting.container_insights.valueas an enum.Resource aws_ecs_cluster
Types of changes
What types of changes does your code introduce to <repo_name>?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...