Skip to content

Conversation

@gberenice
Copy link
Member

@gberenice gberenice commented Dec 30, 2025

what

  • Adds a new variable - uses project_root_prefix globally when set, falling back to existing behavior otherwise.

why

  • Covers a case when the actual path in the repository (e.g., some-dir/root-modules) differs from the root_modules_path, which uses relative paths.
  • No need to set project_root in each stack config, e.g.:
stack_settings:
  project_root: some-dir/root-modules/spacelift-automation

references

  • N/A

Summary by CodeRabbit

Release Notes

  • New Features

    • Added project_root_prefix variable to prepend a prefix to calculated project_root across all stacks
    • Implemented two-tier fallback logic for project_root determination (stack-level settings → prefix → default path)
    • Enhanced AWS integration stack filtering for stricter configuration validation
  • Documentation

    • Updated documentation for the new project_root_prefix input parameter with examples and precedence behavior

✏️ Tip: You can customize this high-level summary in your review settings.

@gberenice gberenice requested a review from a team as a code owner December 30, 2025 16:16
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

📝 Walkthrough

Walkthrough

This pull request introduces a new project_root_prefix variable that allows prepending a prefix to the calculated project_root for all stacks when root_modules_path uses relative paths. The feature includes a two-tier fallback mechanism in the stack configuration merging logic: per-stack YAML-defined project_root takes precedence, followed by the prefix-based calculation, then the default root_modules_path-based derivation. Additionally, the aws_integration_stacks filtering is tightened to check both aws_integration_enabled and resolver availability. Documentation, comprehensive tests, and fixtures are included.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • westonplatter
  • oycyc
  • Gowiem

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 'feat: support prefix for project root' clearly and concisely describes the main feature addition in this pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/project_prefix

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 095c569 and 7ab5d97.

📒 Files selected for processing (6)
  • README.md
  • main.tf
  • tests/fixtures/multi-instance/root-module-a/stacks/custom-project-root.yaml
  • tests/project-root-prefix.tftest.hcl
  • tests/resource-id-resolver.tftest.hcl
  • variables.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.tf
  • main.tf
🧠 Learnings (12)
📓 Common learnings
Learnt from: Gowiem
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: main.tf:56-59
Timestamp: 2024-10-30T17:04:09.798Z
Learning: In this module, `path.root` is intentionally used instead of `path.module` when constructing file paths, as it refers to the root path of the calling module, which is necessary for correctly locating the stack YAML files.
📚 Learning: 2024-10-30T17:11:38.812Z
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/variables.tf:0-0
Timestamp: 2024-10-30T17:11:38.812Z
Learning: In the Terraform module's `variables.tf` file, the variable `autodeploy` defaulting to `true` is acceptable since it matches the default provider value.

Applied to files:

  • variables.tf
📚 Learning: 2024-10-30T17:04:09.798Z
Learnt from: Gowiem
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: main.tf:56-59
Timestamp: 2024-10-30T17:04:09.798Z
Learning: In this module, `path.root` is intentionally used instead of `path.module` when constructing file paths, as it refers to the root path of the calling module, which is necessary for correctly locating the stack YAML files.

Applied to files:

  • tests/fixtures/multi-instance/root-module-a/stacks/custom-project-root.yaml
  • main.tf
📚 Learning: 2024-10-28T12:47:36.579Z
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/variables.tf:54-64
Timestamp: 2024-10-28T12:47:36.579Z
Learning: In the `spacelift-automation` module, the variable `aws_integration_attachment_write` has a default value of `true`, which matches the default value in the provider.

Applied to files:

  • tests/resource-id-resolver.tftest.hcl
  • main.tf
📚 Learning: 2024-10-29T00:06:05.693Z
Learnt from: Gowiem
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/main.tf:221-227
Timestamp: 2024-10-29T00:06:05.693Z
Learning: In the Terraform module `modules/spacelift-automation/main.tf`, when `var.aws_integration_id` is a required variable, avoid suggesting to make the `spacelift_aws_integration_attachment` resource conditional based on whether `aws_integration_id` is provided.

Applied to files:

  • tests/resource-id-resolver.tftest.hcl
  • main.tf
📚 Learning: 2025-09-09T13:21:15.616Z
Learnt from: oycyc
Repo: masterpointio/terraform-spacelift-automation PR: 92
File: data.tf:1-4
Timestamp: 2025-09-09T13:21:15.616Z
Learning: The spacelift_aws_integrations data source exists and is available in the Spacelift Terraform provider for looking up AWS integrations, contrary to previous incorrect analysis.

Applied to files:

  • tests/resource-id-resolver.tftest.hcl
📚 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/resource-id-resolver.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/project-root-prefix.tftest.hcl
📚 Learning: 2024-10-30T17:01:23.897Z
Learnt from: Gowiem
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: main.tf:231-257
Timestamp: 2024-10-30T17:01:23.897Z
Learning: In `main.tf`, grouping related attributes in the `spacelift_stack` resource using locals may overcomplicate things and remove important logic elsewhere; prefer to keep attributes as is.

Applied to files:

  • main.tf
📚 Learning: 2024-10-29T17:52:31.823Z
Learnt from: gberenice
Repo: masterpointio/terraform-spacelift-automation PR: 3
File: modules/spacelift-automation/main.tf:224-230
Timestamp: 2024-10-29T17:52:31.823Z
Learning: In the `masterpointio/terraform-spacelift-automation` repository, the `context.tf` file is a mixin file that we don't own. Do not make suggestions or comments on `context.tf` in future reviews.

Applied to files:

  • main.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:

  • main.tf
📚 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:

  • main.tf
🔇 Additional comments (7)
tests/resource-id-resolver.tftest.hcl (1)

47-47: LGTM! Mock data accurately reflects integration configuration.

The addition of autoattach_enabled = false appropriately models the integration behavior tested by the new filtering logic in main.tf.

README.md (1)

384-384: Excellent documentation for the new feature.

The description clearly explains the use case, behavior, and precedence rules for project_root_prefix. This will help users understand when and how to use this feature effectively.

tests/fixtures/multi-instance/root-module-a/stacks/custom-project-root.yaml (1)

1-9: Well-designed test fixture.

This fixture clearly demonstrates the precedence behavior where per-stack project_root should override the global project_root_prefix. The comment on line 3 helpfully documents the expected behavior.

variables.tf (1)

108-118: Clean variable definition with excellent documentation.

The variable follows Terraform best practices with an appropriate type, optional default, and a comprehensive description that clearly explains the use case and precedence behavior.

main.tf (2)

161-167: Correct implementation of the three-tier project_root fallback.

The logic properly implements the precedence rules:

  1. Per-stack project_root from YAML (highest precedence)
  2. Global project_root_prefix if set
  3. Derived from root_modules_path with "../" stripped (fallback)

The conditional logic and path construction are correct.


414-424: Properly tightened integration filtering logic.

The updated filtering correctly ensures AWS integration attachments are only created when both conditions are met:

  • Integration is enabled
  • Integration ID is resolved (not null)

This prevents attempting to create attachments when using autoattach (where ID would be null). The comment clearly explains this behavior.

tests/project-root-prefix.tftest.hcl (1)

1-151: Excellent test coverage for the new feature.

This test file comprehensively validates the project_root_prefix functionality across multiple scenarios:

  • Default behavior without prefix
  • Prefix application with relative paths
  • Nested directory handling
  • Per-stack precedence rules
  • SingleInstance structure compatibility
  • Fallback behavior replacement

Each test has clear assertions with descriptive error messages, making failures easy to diagnose. Well done!


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
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

Confused and need some clarification. See question below.

# Use specified project_root, if not:
# - If project_root_prefix is set, use project_root_prefix/module
# - Otherwise, use root_modules_path/module with "../" stripped
"project_root" = try(
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm confused while reading this logic: Why is this necessary? if the only difference between using var.project_root_prefix and var.root_modules_path here is that we're replacing "../"... can we just pass var.project_root_prefix without the local directory paths? Does that not accomplish the same thing or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

And let me clarify the above:

Can we just pass var.root_modules_path WITHOUT the ../../ and pass it as terraform/root-modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we can't.

root_modules_path helps to discover sibling root modules and their stacks/*.yaml, while
project_root tells Spacelift where to find the Terraform code in the repo for each stack.

So from spacelift-automation/:

  • ../../root-modules → correctly finds network/, tfstate-backend/, etc. for discovery
  • but Spacelift needs terraform/root-modules/network (from repo root) to run the stack

project_root_prefix bridges this gap:

  • module discovers stacks using ../../root-modules
  • module sets each stack's project_root to terraform/root-modules/<module_name>

I think the root_modules_path name is a bit confusing here. I updated variables description to clarify the difference.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @gberenice, alongside the upcoming major revision to fix stack naming that I'm asking to do as part of #101... I'm thinking that we introduce another variable here that provides what you're looking for and removes the usage of root module's path. If we do that as a breaking change, then we can keep this code cleaner while still providing the functionality that you need here.

What are your thoughts on that? Do you want to continue the conversation live?

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