feat: add remaining VCS integrations #98
Conversation
|
Closing my PR dedicated to BitBucket in favor of this: #97 |
There was a problem hiding this comment.
PR looks good. Hmm I'm just thinking about the tests. I'm not sure how much to justify it because it is mostly just declarative (as there's not much to test, just that namespace = github.lab["namespace"], and it's the same for GitHub, BitBucket, etc. Since the code is kinda just replicating the Terraform Spacelift provider directly, it's not really validating anything I'm thinking?
I guess it doesn't hurt and it's easy to generate, prevents breaking things in the future?
What are your thoughts?
There was a problem hiding this comment.
I had a similar question. I think there are 2 things worth testing,
- That a human correctly added a dynamic around each VCS block blocks
- That the optional
idfield is correctly handled (see thetest_gitlab_integration_id_is_nulltest)
WalkthroughAdds three optional VCS integration inputs (gitlab, bitbucket_cloud, bitbucket_datacenter) to variables.tf and documents them in README.md. In main.tf, introduces corresponding dynamic nested blocks within resource "spacelift_stack" "default", each created only when its input is non-null, setting namespace and id via try(..., null). Adds a new TFT test file validating presence/absence and values of github_enterprise, raw_git, gitlab, bitbucket_cloud, and bitbucket_datacenter blocks using mock providers and plan assertions. No changes to other resources or exported signatures. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
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)
443-535: Prevent multiple VCS blocks from being set simultaneously.Right now, if users pass more than one VCS input, multiple nested blocks will be rendered and the provider will error. Add a precondition to enforce ≤1 selected VCS.
Apply this diff inside resource "spacelift_stack" "default" (e.g., after description or before the first dynamic block):
description = coalesce( try(local.stack_configs[each.key].description, null), try(templatestring(var.description, local.configs[each.key]), null), "Managed by spacelift-automation Terraform root module." ) + lifecycle { + precondition { + condition = sum([ + for v in [ + var.github_enterprise, + var.azure_devops, + var.raw_git, + var.gitlab, + var.bitbucket_cloud, + var.bitbucket_datacenter + ] : v == null ? 0 : 1 + ]) <= 1 + error_message = "Only one VCS integration input may be set (github_enterprise, gitlab, bitbucket_cloud, bitbucket_datacenter, azure_devops, raw_git)." + } + }
🧹 Nitpick comments (3)
variables.tf (1)
57-83: New VCS inputs look correct and align with provider schema.Objects with namespace + optional id mirror existing patterns (GH Enterprise/raw_git) and match Spacelift docs where namespace represents org/group/project across providers. Consider adding a short inline comment clarifying “namespace” semantics for Bitbucket (project) to avoid confusion.
README.md (1)
363-365: Document mutual exclusivity of VCS inputs.Readers can accidentally set multiple VCS blocks at once. Add a note: “At most one of github_enterprise, gitlab, bitbucket_cloud, bitbucket_datacenter, azure_devops, raw_git may be set; zero is allowed (uses account default).”
Also verify the minimum spacelift provider version that supports Bitbucket Data Center and Bitbucket Cloud nested blocks; bump the “Requirements -> Providers” minimum if needed. Would you like me to run a quick web check and propose the exact lower bound?
Also applies to: 381-381
tests/vcs_integrations.tftest.hcl (1)
1-183: Solid coverage; add a negative test for VCS exclusivity.Once the precondition is added, include a failure case to lock behavior.
Apply this additional run block:
run "test_vcs_mutual_exclusivity_violation" { command = plan expect_fail = true variables { github_enterprise = { namespace = "org", id = "gh-id" } gitlab = { namespace = "group", id = "gl-id" } } assert { condition = contains(coalesce(error_message, ""), "Only one VCS integration input may be set") error_message = "Plan should fail when multiple VCS inputs are set." } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(2 hunks)main.tf(1 hunks)tests/vcs_integrations.tftest.hcl(1 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.tf
🧠 Learnings (5)
📓 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: 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.
📚 Learning: 2024-10-23T14:10:26.952Z
Learnt from: oycyc
PR: masterpointio/terraform-aws-ssm-agent#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/vcs_integrations.tftest.hcl
📚 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: 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:
main.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:
main.tf
🔇 Additional comments (3)
main.tf (3)
512-518: GitLab dynamic block: LGTM.Conditional creation and try(...) for id are consistent with provider expectations.
520-526: Bitbucket Cloud dynamic block: LGTM.Matches the established pattern; using namespace here is consistent with Spacelift docs treating it as the workspace/project namespace.
If you’ve seen older configs using “workspace”, confirm provider docs for your pinned version; otherwise this is fine.
528-534: Bitbucket Data Center dynamic block: LGTM.Pattern matches others; namespace maps to Bitbucket project.
Gowiem
left a comment
Gowiem
left a comment
There was a problem hiding this comment.
Sometimes tests are dumb, but that validate that the underlying thing continues to work the way we expect it to work. I'm for it 👍
🤖 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>
What
Why
References
Summary by CodeRabbit
New Features
Documentation
Tests