feat: ability to assign AWS integration by name instead of ID only, and add TF Check blocks for mutual exclusivity #92
Conversation
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
WalkthroughAdds AWS integration name→ID resolution across the module. Introduces data source spacelift_aws_integrations.all and a new input variable aws_integration_name. Refactors main.tf to a generalized resource resolution mechanism (locals for mappings, resolver config, and resolved_resource_ids) covering space, worker_pool, and aws_integration. Updates spacelift_stack to use unified resolved IDs, adds conditional spacelift_aws_integration_attachment, and introduces enablement maps. Schema gains stack_settings.aws_integration_name (mutually exclusive with aws_integration_id). Tests and fixtures are updated to assert the new resolution paths and unified locals. README documents the new data source and input. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.tf (1)
516-523: Fail fast if integration_id is missing on attachmentEven with the check block, a per-resource precondition gives a clearer error at the failing stack.
resource "spacelift_aws_integration_attachment" "default" { for_each = local.aws_integration_stacks integration_id = local.stack_property_resolver[each.key].aws_integration_id stack_id = spacelift_stack.default[each.key].id read = var.aws_integration_attachment_read write = var.aws_integration_attachment_write + lifecycle { + precondition { + condition = local.stack_property_resolver[each.key].aws_integration_id != null + error_message = "AWS integration is enabled for stack '${each.key}' but no integration ID could be resolved. Provide aws_integration_id or aws_integration_name." + } + } }
🧹 Nitpick comments (3)
README.md (1)
328-331: Document resolution precedence and exclusivityConsider adding a short note describing the precedence used by local.resolved_resource_ids: stack ID > stack name > global ID > global name > default. Also call out that aws_integration_id and aws_integration_name are mutually exclusive, and one is required when aws_integration_enabled is true.
Also applies to: 351-351
tests/main.tftest.hcl (1)
1031-1052: Great coverage for AWS integration name resolution; add negative-path tests after enforcing checksOnce mutual exclusivity and “enabled ⇒ id/name required” checks are added, include tests that:
- fail when both aws_integration_id and aws_integration_name are set, and
- fail when aws_integration_enabled is true with neither id nor name set.
I can draft these tftest blocks after you wire the checks.
main.tf (1)
472-479: Use resolved worker_pool_id consistentlyYou still compute worker_pool_id in stack_property_resolver, but the resource correctly uses local.resolved_resource_ids.worker_pool[each.key]. Consider removing the unused worker_pool_id field from stack_property_resolver to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)data.tf(1 hunks)main.tf(4 hunks)stack-config.schema.json(1 hunks)tests/fixtures/multi-instance/root-module-a/stacks/test.yaml(1 hunks)tests/main.tftest.hcl(4 hunks)tests/single-instance.tftest.hcl(2 hunks)variables.tf(1 hunks)
🧰 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.tfmain.tfdata.tf
🧠 Learnings (6)
📓 Common learnings
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#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.
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#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.
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#17
File: examples/single-instance/root-modules/spacelift-automation-example2/main.tf:11-12
Timestamp: 2024-12-23T04:16:47.209Z
Learning: For single-instance Spacelift automation structures, the user prefers to hardcode AWS integration ID and related settings rather than parameterizing them.
📚 Learning: 2024-10-29T00:05:59.989Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#3
File: modules/spacelift-automation/main.tf:215-218
Timestamp: 2024-10-29T00:05:59.989Z
Learning: In the `modules/spacelift-automation/main.tf` file, it's valid to directly reference resource collections created with `for_each` in the `depends_on` attribute. For example, in the `spacelift_stack_destructor` resource, referencing `spacelift_drift_detection.this` and `spacelift_aws_integration_attachment.this` directly is acceptable and should not be flagged.
Applied to files:
README.md
📚 Learning: 2024-10-28T12:47:36.579Z
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#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:
README.mdvariables.tf
📚 Learning: 2024-10-29T00:06:05.693Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#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:
variables.tfmain.tf
📚 Learning: 2024-10-30T17:01:23.897Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#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: 2025-09-03T16:41:42.514Z
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#89
File: main.tf:300-306
Timestamp: 2025-09-03T16:41:42.514Z
Learning: Worker pool names in Spacelift are unique within a Spacelift account, enforced by the platform itself. No additional Terraform validation is needed when creating name-to-ID mappings for worker pools.
Applied to files:
main.tfdata.tf
🔇 Additional comments (5)
tests/fixtures/multi-instance/root-module-a/stacks/test.yaml (1)
5-5: Fixture addition looks goodCovers the new name→ID path. No issues.
variables.tf (1)
106-110: Variable addition LGTMInput is well-described and defaults align with existing pattern.
tests/single-instance.tftest.hcl (1)
148-159: Test updates LGTMAssertions now reference the unified local.resolved_resource_ids.path. Looks good.
tests/main.tftest.hcl (1)
1012-1029: Worker pool resolution assertions LGTMUses the new consolidated map and keeps precedence tests. All good.
main.tf (1)
291-305: Name→ID mappings look correctSpace/worker_pool/AWS integration mappings align with the corresponding data sources.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
README.md (2)
328-331: Docs: Nice addition of the AWS integrations data source.Consider adding a short note that names must exactly match Spacelift’s Integration name and clarify behavior if the name isn’t found (e.g., plan error vs fallback). This avoids silent misconfiguration.
351-352: Docs: Input precedence and mutual exclusivity should be explicit.Please state that aws_integration_id takes precedence over aws_integration_name and that both must not be set together. Mirror the same language you use for worker_pool and space.
tests/main.tftest.hcl (1)
1031-1053: Great addition: AWS integration resolver tests.Consider adding a negative test where aws_integration_enabled = true but name doesn’t resolve, asserting a plan failure once the precondition (see main.tf suggestion) is in place.
main.tf (1)
388-398: Resolution precedence: consider explicit error when a provided name doesn’t resolve.Today a missing name can fall back to globals/default. Optional: fail fast when a stack-level name is set but not found to avoid surprises.
One approach: add per-type checks that if stack_configs[stack][name_attr] is set then it must resolve in name_to_id_mappings. Happy to draft if you want this stricter behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)data.tf(1 hunks)main.tf(4 hunks)stack-config.schema.json(1 hunks)tests/fixtures/multi-instance/root-module-a/stacks/test.yaml(1 hunks)tests/main.tftest.hcl(4 hunks)tests/single-instance.tftest.hcl(2 hunks)variables.tf(1 hunks)
🧰 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.tfmain.tfdata.tf
🧠 Learnings (6)
📓 Common learnings
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#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.
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#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.
📚 Learning: 2024-10-29T00:06:05.693Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#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:
variables.tfmain.tfREADME.md
📚 Learning: 2024-10-28T12:47:36.579Z
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#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:
variables.tfREADME.md
📚 Learning: 2024-10-30T17:01:23.897Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#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: 2025-09-03T16:41:42.514Z
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#89
File: main.tf:300-306
Timestamp: 2025-09-03T16:41:42.514Z
Learning: Worker pool names in Spacelift are unique within a Spacelift account, enforced by the platform itself. No additional Terraform validation is needed when creating name-to-ID mappings for worker pools.
Applied to files:
main.tfdata.tf
📚 Learning: 2024-10-29T00:05:59.989Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#3
File: modules/spacelift-automation/main.tf:215-218
Timestamp: 2024-10-29T00:05:59.989Z
Learning: In the `modules/spacelift-automation/main.tf` file, it's valid to directly reference resource collections created with `for_each` in the `depends_on` attribute. For example, in the `spacelift_stack_destructor` resource, referencing `spacelift_drift_detection.this` and `spacelift_aws_integration_attachment.this` directly is acceptable and should not be flagged.
Applied to files:
README.md
🔇 Additional comments (7)
tests/fixtures/multi-instance/root-module-a/stacks/test.yaml (1)
5-5: LGTM: Fixture exercises name→ID path.Good targeted addition to validate aws_integration_name translation.
tests/single-instance.tftest.hcl (1)
148-160: Tests updated to new resolved_resource_ids shape — looks good.Assertions align with the unified resolver structure.
tests/main.tftest.hcl (2)
851-867: Space resolver test updates: solid conversion.Good move to local.resolved_resource_ids.space; messages now surface the exact map on failure.
1012-1029: Worker pool resolver test updates: on point.References and precedence behavior read cleanly.
main.tf (3)
291-305: Unified name→ID mappings: clear and extensible.This consolidates lookups nicely and keeps iteration cost negligible.
379-387: var_lookup helper: good workaround for dynamic var access.No action needed.
473-479: space_id/worker_pool_id now sourced from unified resolver — nice simplification.Cleaner and easier to extend.
variables.tf
Outdated
|
|
||
| variable "aws_integration_name" { | ||
| type = string | ||
| description = "Name of the AWS integration to attach, which will be resolved to aws_integration_id." |
There was a problem hiding this comment.
I'd add something like "we recommend using name instead of ID for clarity/readability."
There was a problem hiding this comment.
Good point. Added that, also the fact that names must be unique in Spacelift so users don't have to worry about duplicated ones!
oycyc
changed the title
@gberenice do you have thoughts on this? seeing that there's like 70~ lines of local block code for this? |
|
Did a regression test on our internal 
|
🤖 I have created a release *beep* *boop* --- ## [1.8.0](v1.7.0...v1.8.0) (2025-09-18) ### Features * ability to assign AWS integration by name instead of ID only, and add TF Check blocks for mutual exclusivity ([#92](#92)) ([8143a92](8143a92)) * add remaining VCS integrations ([#98](#98)) ([987e24b](987e24b)) --- 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>
Translate and resolve AWS integration names to ID value.
Even though this is more lines of code (because of Terraform programming limitation), I believe this is more readable and maintainable since we avoid having duplicated logic for all values we want to resolve names -> IDs. We now have 3 resources that resolves names into ID, but in the future, we might have more resources that follow similar patterns (e.g. Azure integration ID, etc.)
This will allow a more human readable name in the stack configs rather than ugly IDs
All tests are passing without changing the logic, showing that it is working as intended.
I will follow up with another PR to organize the tests in a separate file dedicated to resolving names -> IDs since the main.tftest.hcl is getting out of hand.
Summary by CodeRabbit
New Features
Documentation