Skip to content

Conversation

@Musthaq101
Copy link
Contributor

@Musthaq101 Musthaq101 commented Oct 28, 2025

what

  • Added provider = aws.config_secrets to data.aws_ssm_parameters_by_path.argocd_notifications
  • Added provider = aws.config_secrets to data.aws_ssm_parameter.github_notifications_app_private_key

why

All encrypted SSM parameter data sources in this component should use the aws.config_secrets provider alias to enable cross-account access to the secrets store.

references

  • Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow).
  • Use closes #123, if this PR closes a GitHub issue #123

Summary by CodeRabbit

  • Chores
    • Updated infrastructure configuration to explicitly scope notification services through the designated secret configuration provider for improved resource isolation and security.

@mergify mergify bot requested review from a team October 28, 2025 07:08
@mergify mergify bot added the triage Needs triage label Oct 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Two Terraform data sources in the notifications configuration are updated to explicitly use the aws.config_secrets provider alias instead of relying on default provider behavior. No functional or logical changes are made.

Changes

Cohort / File(s) Summary
Provider scoping updates
src/notifications.tf
Added explicit provider = aws.config_secrets to two data sources: aws_ssm_parameters_by_path "argocd_notifications" and aws_ssm_parameter "github_notifications_app_private_key"

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify that the aws.config_secrets provider alias is properly defined in the Terraform configuration
  • Confirm that the data sources require this specific provider scope for accessing secrets in a different AWS account or region
  • Check that no other data sources in the module need similar provider scoping adjustments

Poem

🐰 Hopping through the secrets so tight,
Provider aliases now shining bright,
Config secrets scoped with care,
Each data source knows exactly where!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Fixed missing provider configuration for SSM data sources in the notifications module" is concise, specific, and directly reflects the actual changes made in the changeset. The title accurately describes the main change—adding the provider = aws.config_secrets configuration to two SSM data sources in the notifications module—and clearly identifies both the affected resource types (SSM data sources) and the scope (notifications module). The phrasing is clear and avoids vague terminology, making it easy for a teammate reviewing the Git history to understand the primary intent of the change.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@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: 0

🧹 Nitpick comments (2)
src/notifications.tf (2)

1-7: Align provider attribute placement with slack_notifications data source for consistency.

The provider attribute is placed after other arguments (line 6), while the slack_notifications data source (line 10) places it earlier in the block. Per Terraform conventions, meta-arguments like provider and count/for_each should be grouped together early in the block for readability and consistency across the file.

Consider reordering the attributes to place provider immediately after for_each:

data "aws_ssm_parameters_by_path" "argocd_notifications" {
  for_each        = local.notifications_notifiers_ssm_path
-  path            = each.value
-  with_decryption = true
-
  provider = aws.config_secrets
+  path            = each.value
+  with_decryption = true
}

17-23: Align provider attribute placement with slack_notifications data source for consistency.

The provider attribute is placed after other arguments (line 22), while the slack_notifications data source (line 10) places it earlier in the block. Standardize placement by moving provider to appear directly after count:

data "aws_ssm_parameter" "github_notifications_app_private_key" {
  count           = local.github_notifications_enabled && var.github_notifications_app_enabled ? 1 : 0
+  provider        = aws.config_secrets
  name            = var.ssm_github_notifications_app_private_key
  with_decryption = true
-
-  provider = aws.config_secrets
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 065ab5d and 9891620.

📒 Files selected for processing (1)
  • src/notifications.tf (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for all Terraform files
In Terraform, prefer lower_snake_case for variables and locals; keep resource and data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and do not commit formatting violations
Adhere to TFLint rules defined in .tflint.hcl; do not commit lint violations

Files:

  • src/notifications.tf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary

@RoseSecurity
Copy link
Contributor

/terratest

@RoseSecurity
Copy link
Contributor

This looks good to me, but another set of eyes @cloudposse-terraform-components/contributors would be awesome

Copy link
Contributor

@gberenice gberenice left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team October 28, 2025 14:35
@mergify mergify bot removed the triage Needs triage label Oct 28, 2025
@mergify
Copy link

mergify bot commented Oct 28, 2025

Thanks @Musthaq101 for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

@RoseSecurity RoseSecurity added this pull request to the merge queue Oct 28, 2025
Merged via the queue into cloudposse-terraform-components:main with commit 12b6691 Oct 28, 2025
19 checks passed
@github-actions
Copy link

These changes were released in v2.4.0.

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.

3 participants