fix: support TF default workspace, more attributes + fix drift detection schedule #11
Conversation
WalkthroughThe pull request introduces enhancements to the Spacelift automation Terraform module, focusing on updating documentation and improving configuration options. Key modifications include upgrading the Spacelift provider version from Changes
Sequence DiagramsequenceDiagram
participant User
participant SpaceliftModule
participant SpaceliftStack
participant GitHubActions
User->>SpaceliftModule: Configure module
SpaceliftModule->>SpaceliftStack: Create stack
SpaceliftModule->>SpaceliftStack: Set secret masking
SpaceliftModule->>SpaceliftStack: Configure GitHub deploy
GitHubActions->>SpaceliftStack: Deploy via Checks API
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
main.tf (1)
321-321: Consider improving cron schedule validation for drift detectionThe regular expression used for validating the cron schedule may not cover all valid cron expressions and could reject some legitimate schedules or accept invalid ones. For more robust validation, consider using a dedicated cron syntax validation library or module to ensure all valid cron schedules are accurately recognized.
README.md (1)
Line range hint
1-195: Consider adding usage examples for new parametersTo enhance user understanding, consider adding examples demonstrating the usage of the new parameters (
enable_well_known_secret_maskingandgithub_action_deploy) in the main documentation section, similar to other configuration examples.Example addition:
Root module inputs: ```hcl aws_integration_id = "ZDPP8SKNVG0G27T4" + +# Security configuration +enable_well_known_secret_masking = true +github_action_deploy = true # GitHub configuration github_enterprise = { namespace = "masterpointio" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(3 hunks)examples/complete/components/spacelift-automation/stacks/example.yaml(1 hunks)main.tf(2 hunks)outputs.tf(1 hunks)variables.tf(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
outputs.tf (1)
Pattern **/*.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.
variables.tf (1)
Pattern **/*.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.
main.tf (1)
Pattern **/*.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.
🔇 Additional comments (9)
main.tf (1)
253-254: Good addition of configurable secret masking options
Introducing enable_well_known_secret_masking and github_action_deploy enhances the flexibility and security of the Spacelift stack configurations by allowing these settings to be customized per stack.
examples/complete/components/spacelift-automation/stacks/example.yaml (1)
5-5: Drift detection enabled for the example stack
Enabling drift_detection_enabled: true ensures that any unintended changes to the infrastructure are detected promptly, enhancing the reliability and consistency of the stack.
outputs.tf (1)
1-14: Securely exposing essential stack attributes
The new spacelift_stacks output provides important stack details like id, labels, autodeploy, and administrative while carefully avoiding sensitive data exposure. This approach aids in debugging and monitoring without compromising security.
variables.tf (2)
219-223: Addition of enable_well_known_secret_masking variable
Introducing the enable_well_known_secret_masking variable allows users to control the masking of well-known secrets, enhancing security by preventing accidental exposure of sensitive information during runs.
225-228: Addition of github_action_deploy variable
The new github_action_deploy variable provides control over deployment permissions via the GitHub Checks API, offering flexibility for teams using GitHub Actions for continuous deployment workflows.
README.md (4)
128-128: Provider version update looks good!
The Spacelift provider version has been updated from 1.16.1 to 1.19.0, which aligns with the module's requirements of >= 1.14.
177-177: Documentation for new input parameter is clear and concise!
The new enable_well_known_secret_masking parameter is well-documented with a clear description of its purpose and default value.
179-179: Documentation for GitHub action deploy parameter is well-structured!
The new github_action_deploy parameter is properly documented with a clear description and default value set to true.
193-195: New output documentation is clear and security-conscious!
The documentation for spacelift_stacks output clearly explains its purpose and includes an important security note about limiting exported attributes.
variables.tf
Outdated
| variable "enable_well_known_secret_masking" { | ||
| type = bool | ||
| description = "Indicates whether well-known secret masking is enabled." | ||
| default = false |
There was a problem hiding this comment.
This feels like a case where we should build a best practice into the module and default this to true. What do you think?
| default = false | |
| default = true |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
177-177: Consider adding usage examples for new parametersThe documentation for
enable_well_known_secret_maskingandgithub_action_deploywould benefit from examples showing common use cases.Add example configurations in the Usage section:
Root module inputs: ```hcl aws_integration_id = "ZDPP8SKNVG0G27T4" # GitHub configuration github_enterprise = { namespace = "masterpointio" } repository = "terraform-spacelift-automation" # Stacks configurations root_modules_path = "root-modules" enabled_root_modules = ["spacelift-aws-role"] + +# Optional: Configure security features +enable_well_known_secret_masking = true # Enable masking of well-known secrets in logs +github_action_deploy = true # Allow deployments from GitHub ActionsAlso applies to: 179-179 --- `193-195`: **Consider adding example output usage** The new `spacelift_stacks` output would benefit from an example showing how to reference the exported attributes. Add an example in the Usage section: ```diff +## Example Output Usage + +```hcl +output "stack_details" { + description = "Details of created Spacelift stacks" + value = module.spacelift_automation.spacelift_stacks +} +``` + +This will output a map of stacks with their selected attributes, which can be referenced in other parts of your Terraform configuration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(3 hunks)variables.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- variables.tf
🔇 Additional comments (1)
README.md (1)
128-128: Verify compatibility with Spacelift provider version upgrade
The upgrade from 1.16.1 to 1.19.0 is significant. Please ensure all features used in the module are compatible with the new version.
✅ Verification successful
Provider version upgrade appears safe to proceed
The codebase uses standard Spacelift resources (spacelift_stack, spacelift_stack_destructor, spacelift_aws_integration_attachment, spacelift_drift_detection) that have been stable across recent versions. Since no breaking changes were found in the version range and the resources used are core functionality, the upgrade from 1.16.1 to 1.19.0 should be safe.
- Consider adding a version constraint in the provider block to ensure controlled updates in the future
- Test the changes in a non-production environment first
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in the Spacelift provider changelog
gh api repos/spacelift-io/terraform-provider-spacelift/releases --jq '.[] | select(.tag_name | contains("v1.17") or contains("v1.18") or contains("v1.19")) | .body' | grep -i "BREAKING CHANGES\|UPGRADE NOTES"
Length of output: 210
Script:
#!/bin/bash
# Let's try a different approach to check the codebase for potential breaking changes
# First, let's check for any Spacelift provider version references in terraform files
rg -l "provider\s+\"spacelift\"" --type tf
# Then check for actual resource usage
rg "resource\s+\"spacelift_" --type tf -l
# And data source usage
rg "data\s+\"spacelift_" --type tf -l
# Also check for any version constraints in provider blocks
rg "provider\s+\"spacelift\"\s+\{[\s\S]+?version\s*=" --type tf -A 2
Length of output: 201
Script:
#!/bin/bash
# Let's check what Spacelift resources are actually used in main.tf
rg "resource\s+\"spacelift_" -A 2 main.tf
# Also check for any provider block configuration
rg "provider\s+\"spacelift\"" -A 5 main.tf
Length of output: 506
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
main.tf (1)
106-108: Add tests for tfvars file name consistencyThe TODO comment indicates a need for tests to ensure tfvars files are named consistently with stack config files.
Would you like me to help create test cases to verify this naming consistency?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)examples/complete/components/random-pet/stacks/example.yaml(1 hunks)main.tf(5 hunks)variables.tf(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/complete/components/random-pet/stacks/example.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- variables.tf
🧰 Additional context used
📓 Path-based instructions (1)
main.tf (1)
Pattern **/*.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.
🔇 Additional comments (5)
main.tf (4)
105-105: LGTM: Improved workspace handling logic
The updated logic properly handles the default workspace scenario using the try() function.
189-189: LGTM: Consistent folder labeling
The folder label now correctly uses tfvars_file_name, maintaining consistency with the new file naming approach.
255-257: LGTM: Well-structured new attributes
The new attributes are properly implemented with appropriate default handling via try().
324-326: LGTM: Enhanced drift detection validation
The improved precondition now properly validates each schedule individually using alltrue with list comprehension, providing better error handling.
README.md (1)
128-128: LGTM: Comprehensive documentation updates
The README updates accurately reflect all code changes, including:
- Updated provider version
- New attributes documentation
- Output changes
Also applies to: 147-190, 194-196
gberenice
changed the title
|
@Gowiem I've added the commit with changes to support the default TF workspace to this PR. This is a relatively small amount of code to review, so I hope it's okay. |
| "terraform_workspace" = trimsuffix(file, ".yaml"), | ||
| "terraform_workspace" = try(content.default_tf_workspace_enabled, var.default_tf_workspace_enabled) ? "default" : trimsuffix(file, ".yaml"), | ||
| # `yaml` is intentionally used here as we require Stack and `tfvars` config files to be named equally | ||
| # TODO: Add tests to ensure that the `tfvars` file is named the same as the Stack config file |
There was a problem hiding this comment.
This is a good TODO -- One that we should follow up on. I wonder if we can make this a pre-condition?
There was a problem hiding this comment.
@gberenice Ah I had to look up our article on the check block to get a refresher (we're #2 on Google search if you search "terraform check block"!! 🙌 😎), but it looks like this would be a good example to ensure that tfvars + stack yamls both exist. What do you think?
There was a problem hiding this comment.
Hmm, the precondition seems reasonable here; I added one.
Since we operate a lot with stack yaml file content, we don't need to check if it exists, but we should check if a related tfvars file is present. This is crucial for multi-instance model (ha, we're already using this!).
We can add a check to ensure that there is a stack config to each tfvars file. This would throw a warning and won't hault TF execution in case this is expected.
How does that sound to you?
There was a problem hiding this comment.
I think the pre-condition looks great now that I see it / think about it more -- Damn check blocks are confusing.
Let's stick with what you've got! Going to merge so we can share with client 🤝
There was a problem hiding this comment.
@gberenice one suggestion, but otherwise, this looks great and ready to ship! ⚡
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
main.tf (1)
280-290: Consider enhancing validation with a check blockWhile the precondition correctly validates tfvars file existence, consider implementing this as a check block to validate both stack YAML and tfvars files together, as suggested in previous discussions.
Example enhancement:
+ check "configuration_files" { + assert { + condition = fileexists("${local.configs[each.key].project_root}/tfvars/${local.configs[each.key].tfvars_file_name}.tfvars") + error_message = "The required .tfvars file is missing for stack \"${each.key}\"." + } + assert { + condition = fileexists("${local.configs[each.key].project_root}/stacks/${local.configs[each.key].tfvars_file_name}.yaml") + error_message = "The required stack YAML file is missing for stack \"${each.key}\"." + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)examples/complete/components/random-pet/stacks/common.yaml(1 hunks)examples/complete/components/random-pet/stacks/example.yaml(1 hunks)main.tf(5 hunks)variables.tf(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/complete/components/random-pet/stacks/common.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/complete/components/random-pet/stacks/example.yaml
- variables.tf
🧰 Additional context used
📓 Path-based instructions (1)
main.tf (1)
Pattern **/*.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.
🔇 Additional comments (5)
main.tf (4)
105-108: LGTM: Workspace handling improvements
The changes effectively support the default Terraform workspace while maintaining backward compatibility. The addition of tfvars_file_name improves the consistency between stack and variable files.
255-257: LGTM: New Spacelift features properly integrated
The new attributes enable_well_known_secret_masking and github_action_deploy are correctly implemented with proper fallback behavior.
339-341: LGTM: Improved drift detection schedule validation
The new validation approach correctly handles multiple schedules and properly validates the cron format for each entry, fixing the reported precondition error.
189-189: LGTM: Consistent folder labeling
The folder label now consistently uses tfvars_file_name, aligning with the new naming convention.
README.md (1)
147-196: LGTM: Comprehensive documentation updates
The README updates thoroughly document all new features and changes, including:
- Updated provider version
- New input parameters with clear descriptions
- New output documentation
!
what
This adds:
defaultTF workspace while keeping the required configs structure.why
references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
enable_well_known_secret_masking,github_action_deploy, anddefault_tf_workspace_enabled.spacelift_stacksto minimize the risk of exporting sensitive data.drift_detection_enabled.Bug Fixes
Documentation