Skip to content

Conversation

@Musthaq101
Copy link
Contributor

@Musthaq101 Musthaq101 commented Oct 14, 2025

what

  • Consistent with codebase pattern: Other SSM parameter reads in data.tf and notifications.tf already use the same provider = aws.config_secrets pattern
  • Proper provider alias: The aws.config_secrets provider is defined in provider-secrets.tf and configured to access SSM parameters from a potentially different account/region
  • Correct for secrets management: GitHub credentials should be read from the designated secrets store account, not the default provider region

why

  • The v2.2.0 component has a bug where it doesn't specify the provider for reading GitHub API key.
  • Other SSM parameters (like OIDC, deploy keys, notifications) correctly use provider = aws.config_secrets, but the GitHub API key was missing this line.

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

  • Bug Fixes

    • Improved reliability of secret retrieval by explicitly using the correct configuration for secure parameters, reducing intermittent failures across environments.
  • Chores

    • Standardized infrastructure configuration for fetching secrets to ensure consistency across accounts and regions. No user-facing behavior changes.

data.tf
 and
notifications.tf
 already use the same provider = aws.config_secrets pattern
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

Walkthrough

Adds a provider alias override (aws.config_secrets) to two AWS SSM Parameter data sources in src/provider-github.tf. No other logic or flow changes.

Changes

Cohort / File(s) Summary of Changes
Terraform provider aliasing
src/provider-github.tf
Added provider = aws.config_secrets to data "aws_ssm_parameter" "github_api_key" and data "aws_ssm_parameter" "github_app_private_key"; no other modifications.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny taps the Terraform keys,
Points secrets to the right SSM breeze.
No flows rerouted, no extra hops—
Just whisper-paths to safer stops.
Thump-thump, approved! Now onward, please. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly describes the core issue and the fix by indicating that the component is missing the provider specification when reading the GitHub API key from SSM, which directly reflects the main change in the pull request.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ 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.

@mergify mergify bot requested review from a team October 14, 2025 09:05
@mergify mergify bot added the triage Needs triage label Oct 14, 2025
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: 1

🧹 Nitpick comments (1)
src/provider-github.tf (1)

51-53: Optional: avoid unnecessary SSM read when token override is provided

If github_token_override is set, you can skip the SSM fetch to reduce external calls.

Apply:

 data "aws_ssm_parameter" "github_api_key" {
-  count           = !var.github_app_enabled ? 1 : 0
+  count           = (!var.github_app_enabled && var.github_token_override == null) ? 1 : 0
   name            = var.ssm_github_api_key
   with_decryption = true
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0b24cb and dcb9ab9.

📒 Files selected for processing (1)
  • src/provider-github.tf (1 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/provider-github.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
🔇 Additional comments (1)
src/provider-github.tf (1)

63-65: Also correct: alias for GitHub App private key

Using aws.config_secrets here is consistent and necessary. LGTM.

Please confirm the alias is configured (provider "aws" { alias = "config_secrets" ... }) and points to the secrets account/region.

@RoseSecurity
Copy link
Contributor

/terratest

Copy link
Contributor

@RoseSecurity RoseSecurity left a comment

Choose a reason for hiding this comment

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

This is a great finding. Thank you

@RoseSecurity RoseSecurity added this pull request to the merge queue Oct 14, 2025
@mergify mergify bot requested a review from a team October 14, 2025 12:50
@mergify mergify bot removed the triage Needs triage label Oct 14, 2025
@mergify
Copy link

mergify bot commented Oct 14, 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.

Merged via the queue into cloudposse-terraform-components:main with commit 065ab5d Oct 14, 2025
19 checks passed
@github-actions
Copy link

These changes were released in v2.3.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.

2 participants