Skip to content

CAB-4856: Apply master-branch kno2-discharge-summary Terraform and check for drift#38

Merged
irsanchez merged 2 commits intopp_masterfrom
CAB-4856-logvars
Oct 2, 2025
Merged

CAB-4856: Apply master-branch kno2-discharge-summary Terraform and check for drift#38
irsanchez merged 2 commits intopp_masterfrom
CAB-4856-logvars

Conversation

@irsanchez
Copy link
Copy Markdown

@irsanchez irsanchez commented Oct 2, 2025

Link to Ticket: https://hbuco.atlassian.net/browse/CAB-4856

Summary by CodeRabbit

  • New Features

    • Added an optional setting to control whether the module manages the CloudWatch log group.
    • Log subscription to Sumo Logic now only configures when both log group management and log shipping are enabled.
  • Bug Fixes

    • Preserve existing log groups on destroy to avoid accidental deletion.
  • Notes

    • No changes to existing Lambda runtime behavior; defaults preserve current functionality.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 2, 2025

Walkthrough

Adds a new boolean variable manage_log_group. Conditionally creates aws_cloudwatch_log_group when enabled, adjusts aws_cloudwatch_log_subscription_filter to use either the managed log group name or a local value based on manage_log_group, and keeps subscription creation tied to ship_logs_to_sumo. Adds skip_destroy on the log group.

Changes

Cohort / File(s) Summary
Conditional log group and subscription filter
lambda_function/lambda_function.tf
Adds conditional creation of aws_cloudwatch_log_group using count = var.manage_log_group ? 1 : 0; adds/uses skip_destroy on the log group; makes aws_cloudwatch_log_subscription_filter creation conditional on var.ship_logs_to_sumo and resolves log_group_name with a conditional expression: when managed use aws_cloudwatch_log_group.lambda_log_group[0].name, otherwise use local.log_group_name.
New configuration variable
lambda_function/variables.tf
Adds variable "manage_log_group" { type = bool, default = true } to let callers opt out of module-managed CloudWatch log group.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant U as User
    participant T as Terraform
    participant V as var.manage_log_group
    participant S as var.ship_logs_to_sumo
    participant LG as aws_cloudwatch_log_group
    participant SF as aws_cloudwatch_log_subscription_filter
    participant L as local.log_group_name

    U->>T: plan/apply
    T->>V: read manage_log_group
    alt manage_log_group == true
        T-->>LG: create (count=1, skip_destroy)
        note right of LG#lightblue: Managed log group
        alt ship_logs_to_sumo == true
            T-->>SF: create with log_group_name = LG[0].name
        else ship_logs_to_sumo == false
            T--x SF: do not create
        end
    else manage_log_group == false
        T--x LG: do not create
        alt ship_logs_to_sumo == true
            T-->>SF: create with log_group_name = L
        else ship_logs_to_sumo == false
            T--x SF: do not create
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • smallesh90

Poem

A twitch of whiskers, toggles flip—
I burrow into Terraform’s script.
Logs sprout when flags say “grow,”
Filters hop when sumo winds blow.
Thump-thump: apply, and off I go. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title focuses on applying a “kno2-discharge-summary” Terraform change and drift checking, which does not describe the actual updates in this PR that introduce a manage_log_group variable and conditional CloudWatch log group and subscription filter logic. It therefore misrepresents the core changes. Update the title to succinctly reflect the key change, for example “Add manage_log_group flag and conditional CloudWatch log group creation for Lambda” to clearly convey the primary update.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch CAB-4856-logvars

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 878bc93 and dfc19e2.

📒 Files selected for processing (1)
  • lambda_function/lambda_function.tf (2 hunks)

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 18782b7 and 878bc93.

📒 Files selected for processing (2)
  • lambda_function/lambda_function.tf (1 hunks)
  • lambda_function/variables.tf (1 hunks)

Comment thread lambda_function/lambda_function.tf Outdated
@irsanchez irsanchez merged commit a8f7391 into pp_master Oct 2, 2025
1 check passed
@irsanchez irsanchez deleted the CAB-4856-logvars branch October 2, 2025 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants