fix!: file should be optional + actualize docs - #28
Conversation
WalkthroughThis pull request refactors the secrets management system to support multiple providers instead of SOPS alone. The implementation is reorganized by moving the secrets module configuration to a new mixin file ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
13-13: Consider updating the intro text for consistency.Line 13 still describes the module as being "built to handle SOPS secrets" with future extensibility, but line 75 now states it "currently supports SOPS and AWS SSM Parameter Store." Consider updating this intro paragraph to reflect that SSM support is already implemented.
🔎 Suggested update
-Our initial version is built to handle [SOPS secrets](https://github.com/getsops/sops), but it is designed in a way that it can be easily extended to support other secret providers like AWS SSM Parameter Store, Vault, and more in the future. +The module currently supports [SOPS secrets](https://github.com/getsops/sops) and AWS SSM Parameter Store, and is designed to be easily extended to support additional secret providers like HashiCorp Vault, AWS Secrets Manager, and more in the future.exports/secrets.mixin.tf (1)
16-22: Consider aligning thetypefield definition withvariables.tf.In this mixin,
typeis defined as a requiredstring(line 19), whereasvariables.tfdefines it asoptional(string, "sops"). This means consumers using the mixin must explicitly specifytypefor every secret, while the module itself defaults to"sops".If backward compatibility is important (existing configs omit
type), consider making this consistent:- type = string + type = optional(string, "sops")Otherwise, if requiring explicit types is intentional for clarity, the current approach is fine — just ensure the README examples reflect that
typeis required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.mdexports/secrets.mixin.tfexports/secrets.sops.tftests/locals.tftest.hcltests/outputs.tftest.hclvariables.tf
💤 Files with no reviewable changes (1)
- exports/secrets.sops.tf
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tf
⚙️ CodeRabbit configuration file
**/*.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.
Files:
variables.tfexports/secrets.mixin.tf
🧠 Learnings (7)
📚 Learning: 2024-10-23T18:20:57.022Z
Learnt from: oycyc
Repo: masterpointio/terraform-aws-ssm-agent PR: 28
File: tests/unit.tftest.hcl:34-71
Timestamp: 2024-10-23T18:20:57.022Z
Learning: In the `terraform-aws-ssm-agent` module, the user prefers not to include additional assertions for network and security configurations (e.g., root volume encryption, network interface configurations, and security group associations) in the `verify_launch_template` unit test in `tests/unit.tftest.hcl`.
Applied to files:
tests/locals.tftest.hcl
📚 Learning: 2024-11-21T13:30:01.588Z
Learnt from: gberenice
Repo: masterpointio/terraform-aws-tailscale PR: 41
File: main.tf:0-0
Timestamp: 2024-11-21T13:30:01.588Z
Learning: In this Terraform module (`main.tf`), read permissions (`ssm:GetParameter`) for SSM parameters are managed by the SSM Agent module (`masterpointio/ssm-agent/aws`), so adding `ssm:GetParameter` permissions in the custom `ssm_policy` module is unnecessary.
Applied to files:
README.mdexports/secrets.mixin.tf
📚 Learning: 2025-05-12T15:44:10.001Z
Learnt from: gberenice
Repo: masterpointio/terraform-users-groups-automation-googleworkspace PR: 1
File: examples/import-existing-org/imports.tf:7-7
Timestamp: 2025-05-12T15:44:10.001Z
Learning: In Terraform, local variables defined in one .tf file are available throughout all other .tf files in the same module directory, which allows for referencing locals across different files.
Applied to files:
exports/secrets.mixin.tf
📚 Learning: 2024-10-23T14:10:29.597Z
Learnt from: oycyc
Repo: masterpointio/terraform-aws-ssm-agent PR: 28
File: tests/main.tftest.hcl:27-28
Timestamp: 2024-10-23T14:10:29.597Z
Learning: In Terraform test files (e.g., `tests/main.tftest.hcl`), module outputs can be accessed without assigning an explicit name to the module.
Applied to files:
exports/secrets.mixin.tftests/outputs.tftest.hcl
📚 Learning: 2024-11-18T12:59:36.156Z
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-aws-integrations PR: 2
File: main.tf:2-8
Timestamp: 2024-11-18T12:59:36.156Z
Learning: In this Terraform configuration, `module.this` is defined in `context.tf`.
Applied to files:
exports/secrets.mixin.tf
📚 Learning: 2024-11-07T21:31:09.849Z
Learnt from: Gowiem
Repo: masterpointio/terraform-aws-ssm-agent PR: 28
File: tests/main.tftest.hcl:14-46
Timestamp: 2024-11-07T21:31:09.849Z
Learning: In `tftest.hcl` files, `export_outputs` is not a valid attribute within a `run` block.
Applied to files:
tests/outputs.tftest.hcl
📚 Learning: 2024-10-23T14:10:26.952Z
Learnt from: oycyc
Repo: masterpointio/terraform-aws-ssm-agent PR: 28
File: tests/cpu-compatibility.tftest.hcl:1-11
Timestamp: 2024-10-23T14:10:26.952Z
Learning: In `tests/cpu-compatibility.tftest.hcl`, the variables block contains mock values intended for testing purposes.
Applied to files:
tests/outputs.tftest.hcl
🔇 Additional comments (8)
tests/outputs.tftest.hcl (2)
74-96: LGTM! Well-structured SSM-only test case.The test properly validates SSM secret retrieval with appropriate assertions for both the secret value and count. Good use of the mock provider defined earlier.
98-130: LGTM! Comprehensive mixed sources test.This test effectively validates the core functionality being introduced in this PR — the ability to combine SOPS and SSM secrets in a single configuration. The assertions properly verify both secret types resolve correctly.
tests/locals.tftest.hcl (2)
91-132: LGTM! Thorough SSM locals validation.Good coverage of the derived local variables (
ssm_secret_mapping,ssm_paths,ssm_secrets). The negative assertion at line 129 verifyingsops_secret_mappingis empty is a nice touch to ensure proper filtering.
134-180: LGTM! Solid mixed configuration test.This test effectively validates that SOPS and SSM secrets are properly separated into their respective mappings while still being merged into the unified
local.secretsmap. Good assertion on the final merged keys.README.md (1)
44-71: LGTM! Clear documentation for new SSM and mixed source features.The examples effectively demonstrate both SSM-only and mixed source configurations. The documentation aligns well with the variable definition and validation rules.
variables.tf (1)
16-38: LGTM! Well-designed validations.The three-tier validation approach (valid type → SOPS requires file → SSM requires path) provides clear, actionable error messages. Using
alltruewith list comprehensions is idiomatic Terraform for validating lists of objects. As per Terraform best practices, this fail-fast approach prevents runtime errors.exports/secrets.mixin.tf (2)
11-12: Verify the module version is correct for this release.The mixin references
version = "1.1.0", but this PR is introducing the SSM support. Ensure this version matches what will be released, or update it accordingly when the new version is published.
1-14: LGTM! Clean mixin structure.The approach of providing a copyable mixin with tflint/checkov annotations is a good developer experience pattern. The local alias
secrets = module.secrets.allprovides the expectedlocal.secrets["name"]access pattern documented in the README.
Gowiem
left a comment
There was a problem hiding this comment.
@gberenice this looks awesome, but we may want to rev minor or major since we're 1) adding variable validation and 2) renaming the mixin file (maybe a reason to major rev?).
What do you think?
Makes sense. PR title updated. |
🤖 I have created a release *beep* *boop* --- ## [2.0.0](v1.0.0...v2.0.0) (2025-12-23) ### ⚠ BREAKING CHANGES * file should be optional + actualize docs ([#28](#28)) ### Bug Fixes * file should be optional + actualize docs ([#28](#28)) ([3a4a8e6](3a4a8e6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: masterpointbot[bot] <177651640+masterpointbot[bot]@users.noreply.github.com>
what
fileoptional for SSM secrets - SSM secrets only requirepath, SOPS secrets only requirefile"sops"or"ssm"fileattributepathattributesecrets.mixin.tf- Single unified mixin file replacing separatesecrets.sops.tfandsecrets.ssm.tfwhy
fileattribute was required even for SSM secrets.references
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.