Add workspace checks, default handling & config flag #1073
Conversation
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis pull request updates workspace management in Atmos’s Terraform integration. It adds a conditional check in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (4)
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/docs/core-concepts/components/terraform/workspaces.mdx (1)
239-239: Add a comma for better readabilityThe sentence structure would be improved with a comma before "since".
-Setting `workspaces_enabled: true` for an HTTP backend will be ignored with a warning message since HTTP backends don't support workspaces. +Setting `workspaces_enabled: true` for an HTTP backend will be ignored with a warning message, since HTTP backends don't support workspaces.🧰 Tools
🪛 LanguageTool
[uncategorized] ~239-~239: Possible missing comma found.
Context: ... backend will be ignored with a warning message since HTTP backends don't support works...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/exec/stack_utils.go(1 hunks)internal/exec/stack_utils_test.go(1 hunks)internal/exec/terraform.go(2 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/terraform_utils_test.go(1 hunks)pkg/schema/schema.go(1 hunks)website/docs/core-concepts/components/terraform/workspaces.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/components/terraform/workspaces.mdx
[uncategorized] ~239-~239: Possible missing comma found.
Context: ... backend will be ignored with a warning message since HTTP backends don't support works...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/schema/schema.go (1)
130-130: Well-designed field addition for workspace configuration.The new
WorkspacesEnabledfield is appropriately defined as a pointer to a boolean to handle the tri-state logic (explicitly enabled, explicitly disabled, or not set). The use ofomitemptytag is correct for an optional configuration parameter.internal/exec/terraform.go (2)
26-27: Clear and descriptive error variable.Good error message that clearly explains the limitation.
111-114: Effective check preventing unsupported operation.This check correctly prevents workspace commands from being used with HTTP backends, which don't support workspaces. The error is specific and informative.
internal/exec/terraform_utils.go (1)
148-170: Well-implemented function for workspace status determination.This function effectively implements the workspace enablement logic with proper handling of HTTP backends and explicit configuration settings. The warning when attempting to enable workspaces with HTTP backend is particularly helpful for users.
The function's implementation follows good practices:
- Checks HTTP backend first (which overrides other settings)
- Then checks for explicit configuration
- Falls back to the default behavior
internal/exec/terraform_utils_test.go (2)
10-13: Useful helper function for creating boolean pointers in tests.Clean implementation that simplifies the test code.
15-84: Comprehensive test coverage for the workspace enablement logic.Excellent test design that covers all important scenarios:
- Default behavior
- HTTP backend auto-disabling
- Explicit enablement/disablement
- Warning case for HTTP backend
The test structure is clean, well-organized, and follows best practices with descriptive test case names and clear assertions.
internal/exec/stack_utils.go (1)
15-20: Good job handling disabled workspaces!The added code correctly detects when workspaces are disabled and returns the default workspace. This improves compatibility with backends that don't support workspaces (like HTTP backend) as mentioned in the PR description.
The comments are clear and explain well why this change is needed. Note that this matches a previous suggestion from osterman in an earlier review.
internal/exec/stack_utils_test.go (1)
10-98: Good test coverage for workspace handling scenariosThe test cases are thorough and cover all important scenarios:
- Default behavior with workspaces enabled
- HTTP backend automatically disabling workspaces
- Explicitly enabled/disabled workspaces
- Edge cases like empty stack name
Your test cases validate both the functionality and the edge cases of the new feature.
One small issue - there's a missing helper function:
+ // boolPtr returns a pointer to the given boolean value + func boolPtr(b bool) *bool { + return &b + }This function is used in the tests but isn't defined in the visible code.
website/docs/core-concepts/components/terraform/workspaces.mdx (1)
200-252: Well-documented feature with clear examplesThe new documentation section provides clear explanations for:
- When workspaces are disabled automatically
- How to explicitly disable workspaces
- The implications of disabling workspaces
- Scenarios when disabling workspaces makes sense
This is comprehensive and helpful for users to understand the new feature.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~239-~239: Possible missing comma found.
Context: ... backend will be ignored with a warning message since HTTP backends don't support works...(AI_HYDRA_LEO_MISSING_COMMA)
aknysh
added
the
no-release
aknysh
dismissed stale reviews from coderabbitai[bot] and themself
via
3340640
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
aknysh
added
minor
|
These changes were released in v1.164.0. |
what
This simple check if ComponentBackendType is a backend server which is not supported by atmos workspaces and disable the command
why
references
Summary by CodeRabbit
New Features
Documentation
Tests
Chores