feat: stack validation improvements #4078
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis PR introduces new Terragrunt configuration files defining units and stacks along with corresponding Terraform files. It adds integration tests for error handling during stack generation and updates the stack generation logic in the configuration package by introducing component categorization and target directory validation. A new command-line flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as CLI User
participant CLI as Stack CLI Command
participant G as GenerateStacks Function
participant V as Validator
participant W as Worker Pool
U->>CLI: Execute "terragrunt stack generate"
CLI->>CLI: Parse flags (including NoStackValidate)
CLI->>G: Invoke stack generation
G->>V: Validate target directories (if validation enabled)
V-->>G: Return validation results
G->>W: Submit tasks for generating stacks/units concurrently
W-->>G: Return task results/errors
G-->>CLI: Return final generation result
CLI-->>U: Display output/errors
sequenceDiagram
participant C as Component Processor
participant WP as Worker Pool
participant T as Task Executor
C->>WP: Submit task for processing component
WP->>T: Start task in a new goroutine
T-->>WP: Return task result or error
WP->>WP: Collect and aggregate errors
WP-->>C: Signal completion via Wait
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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
|
|
@denis256 this is good, but it doesn't actually validate that the unit has a |
![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
🧹 Nitpick comments (4)
internal/worker/worker.go (4)
1-19: Great package-level documentation; consider adding an example use case for clarity.These doc comments are detailed and outline the functionality well. However, adding a minimal code snippet demonstrating usage would help new users quickly grasp how to integrate and run tasks with this worker pool.
29-46: Consider unifying concurrency handling forisRunningandisStopping.Currently,
isStoppingis managed by anatomic.Bool, whileisRunningis protected by anRWMutex. Mixing approaches can introduce complexity. Consolidating both fields under either an atomic or a mutex-based approach might simplify the code and reduce potential confusion. For instance, you could do:- isRunning bool + isRunning atomic.BoolThen remove or adapt the associated locks accordingly where
isRunningis updated.
95-119: Consider streamlining error storage to avoid duplication.Currently, errors are appended to both
wp.allErrorsandwp.errorsSlice. This maintains backward compatibility but creates overhead. If possible, refactor to rely solely onwp.allErrorsand derive any slice representations on-demand.
190-216:Stop()is asynchronous; clarify or rename to better reflect its behavior.Because
Stop()returns immediately, concurrency newcomers might expect it to block. Consider documenting that it initiates a shutdown and proceeds asynchronously, or rename it (e.g.,StopAsync) to reduce confusion. You could also add something like:-// Stop shuts down the worker pool after current tasks are completed +// Stop initiates a shutdown and returns immediately, without blocking for in-flight tasks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/worker/worker.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
internal/worker/worker.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (4)
internal/worker/worker.go (4)
48-63: Boundary handling for zero or negative workers looks good.Automatically elevating
maxWorkersto 1 if a non-positive value is provided prevents unforeseen panics and ensures a functional pool. This approach appears solid for most use cases.
65-93: Channel re-initialization logic inStart()is concise and correct.Recreating channels and clearing errors upon restart prevents lingering state or stale data from a previous run.
132-174:Submit()respects previous feedback on RWLock usage.This code now uses
RLockto guardisRunning, addressing the earlier suggestion by yhakbar regarding potential efficiency gains.
218-237:GracefulStop()correctly completes all tasks before shutting down.This blocking approach is well-structured, ensuring the pool finalizes work properly and avoids channel panics.
denis256
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/_docs/04_reference/02-cli-options.md (1)
359-365: Enhance Clarity of the Stack Validation SectionThe new bullet clearly explains that during stack generation Terragrunt validates that each unit’s directory contains a
terragrunt.hclfile and that each stack’s directory contains aterragrunt.stack.hclfile. However, the flow could be improved for better readability and consistency with the rest of the documentation. For example, consider rephrasing the section to explicitly mention Terragrunt and tighten the language. A suggested diff is provided below:- - Validation of Units and Stacks: During the stack generation, the system will validate that each unit and stack's target directory contains the appropriate configuration file (`terragrunt.hcl` for units and `terragrunt.stack.hcl` for stacks). This ensures the directories are correctly structured before proceeding with the stack generation. - To **skip this validation**, you can use the `--no-stack-validate` flag: - - ```bash - terragrunt stack generate --no-stack-validate - ``` + - **Validation of Units and Stacks**: During stack generation, Terragrunt validates that each unit’s target directory contains a `terragrunt.hcl` file and that each stack’s target directory contains a `terragrunt.stack.hcl` file. This check ensures the directory structure is correct before proceeding. + To skip this validation, use the `--no-stack-validate` flag: + + ```bash + terragrunt stack generate --no-stack-validate + ```This minor rewording improves clarity and aligns the description with the overall documentation style.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/config.go(0 hunks)docs/_docs/04_reference/02-cli-options.md(1 hunks)options/options.go(2 hunks)
💤 Files with no reviewable changes (1)
- config/config.go
🚧 Files skipped from review as they are similar to previous changes (1)
- options/options.go
🧰 Additional context used
📓 Path-based instructions (1)
`docs/**/*.md`: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration under...
docs/**/*.md: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation indocsto the Starlight + Astro based documentation indocs-starlight. Whenever changes are made to thedocsdirectory, ensure that an equivalent change is made in thedocs-starlightdirectory to keep thedocs-starlightdocumentation accurate.
docs/_docs/04_reference/02-cli-options.md
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/fixtures/stacks/errors/cycles/live/terragrunt.stack.hcl (1)
1-4: Clear and Concise Stack Declaration:
The consolidation into a single "stack" declaration simplifies the configuration and aligns with the intended validation improvements. The use of the expression${get_repo_root()}/stackin thesourcevalue promotes portability across test environments. Please confirm that the helper functionget_repo_root()is defined and available in your testing context, ensuring it returns the correct repository root path. Additionally, verify that the integration tests fully exercise this configuration scenario to catch any missing validations (e.g., ensuring that aterragrunt.stack.hclfile is indeed detected as expected).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/fixtures/stacks/errors/cycles/live/terragrunt.stack.hcl(1 hunks)test/fixtures/stacks/errors/cycles/stack/terragrunt.stack.hcl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/stacks/errors/cycles/stack/terragrunt.stack.hcl
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unessential
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
Description
Demo:
RFC: #3313
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
--no-stack-validate).Documentation
Refactor