-
Notifications
You must be signed in to change notification settings - Fork 3
feat: ability to assign workerpool by name #89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a Spacelift worker pools data source in data.tf. Introduces a new input variable worker_pool_name in variables.tf and clarifies mutual exclusivity with worker_pool_id; both default to null. In main.tf, creates a worker_pool_name_to_id map and a resolved_worker_pool_ids local that prioritizes per-stack worker_pool_id, then per-stack worker_pool_name, then global worker_pool_id, then global worker_pool_name. Updates spacelift_stack.default to use resolved_worker_pool_ids for worker_pool_id. Existing data sources and schema validation remain unchanged. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
data.tf (1)
4-6: Data source added — consider scope and intentLooks good. If the PR intent is “private workers only,” ensure downstream mapping filters to private pools; otherwise this will include public/default pools as well.
Would you like me to add a small filter in the mapping (see main.tf comment) once we confirm the provider attribute that denotes “private”?
variables.tf (2)
353-357: Good clarification on mutual exclusivityDocs look clear. Enforcement currently lives only in prose; see suggested check blocks in main.tf to hard-enforce this at plan time.
I can open a follow-up PR adding those checks if you’d like.
359-366: New variable reads wellDefinition and defaults are appropriate. Pair with the plan-time checks in main.tf for a complete UX.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
data.tf(1 hunks)main.tf(3 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:
data.tfmain.tfvariables.tf
🧠 Learnings (3)
📓 Common learnings
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#3
File: modules/spacelift-automation/variables.tf:235-242
Timestamp: 2024-10-29T17:54:26.382Z
Learning: In `modules/spacelift-automation/variables.tf`, the `worker_pool_id` variable does not follow the UUID format.
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#3
File: variables.tf:245-251
Timestamp: 2024-10-30T17:05:18.442Z
Learning: When users are expected to know certain requirements, such as `worker_pool_id` being required for self-hosted Spacelift instances, avoid adding additional variables solely for validation purposes.
📚 Learning: 2024-10-29T17:54:26.382Z
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#3
File: modules/spacelift-automation/variables.tf:235-242
Timestamp: 2024-10-29T17:54:26.382Z
Learning: In `modules/spacelift-automation/variables.tf`, the `worker_pool_id` variable does not follow the UUID format.
Applied to files:
main.tfvariables.tf
📚 Learning: 2024-10-30T17:05:18.442Z
Learnt from: Gowiem
PR: masterpointio/terraform-spacelift-automation#3
File: variables.tf:245-251
Timestamp: 2024-10-30T17:05:18.442Z
Learning: When users are expected to know certain requirements, such as `worker_pool_id` being required for self-hosted Spacelift instances, avoid adding additional variables solely for validation purposes.
Applied to files:
variables.tf
🔇 Additional comments (1)
main.tf (1)
455-455: Stack wiring LGTMUsing local.resolved_worker_pool_ids here aligns with the new resolution flow.
gberenice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jgeissler14 looks good!
A couple of suggestions + since we're adding one more complex local expression, could you please add TF tests to check if a correct worker pool ID is selected?
Gowiem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small things 👍
|
Thanks for the PR Josh! Pushing this PR through -- committed some updates here to address PR reviews and added tests. No need to test existing worker ID behavior as that already has coverage. So testing new behavior with worker_pool_name and ensuring that configs in stack.yaml takes precedence in the global variable. |
gberenice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for moving this ahead, Yang ❤️
🤖 I have created a release *beep* *boop* --- ## [1.7.0](v1.6.0...v1.7.0) (2025-09-15) ### Features * ability to assign workerpool by name ([#89](#89)) ([7597e4d](7597e4d)) * enable integration with ADO ([#93](#93)) ([f3b0da4](f3b0da4)) --- 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
This change will allow a user to assign a workerpool to a stack by either workerpool name or id, mutually exclusive. The name can be passed as a var and then compared to the data block pulling all private workers to match name=name, and grab the id for reference later in the codebase.
All tf tests passed
why
It can be confusing to read when only looking at ids to know which it is referencing. This should help prevent these problems in the future.
references
Summary by CodeRabbit
New Features
Enhancements
Documentation