feat: Implementation of --all --graph flags
#3944
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis PR refactors several Terragrunt CLI commands by introducing additional wrapping layers using the Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Cmd as Command Object
participant RunAll as runall.WrapCommand
participant Graph as graph.WrapCommand
participant Action as Underlying Command Action
User->>Cmd: Invoke Command
Cmd->>RunAll: Wrap command for stack execution
RunAll->>Graph: Enhance command with graph behavior
Graph->>Action: Execute wrapped command action
Possibly related PRs
Suggested reviewers
Poem
✨ 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
CodeRabbit Configuration File (
|
![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 (2)
cli/commands/common/runall/command.go (1)
18-19: Redundant constant definitions detectedThe constants
DeprecatedOutDirFlagNameandOutDirFlagNamehave identical values ("out-dir"), and similarly for the JSON versions. This appears redundant and could cause confusion for maintainers.-const ( - AllFlagName = "all" - OutDirFlagName = "out-dir" - JSONOutDirFlagName = "json-out-dir" - - DeprecatedOutDirFlagName = "out-dir" - DeprecatedJSONOutDirFlagName = "json-out-dir" -) +const ( + AllFlagName = "all" + OutDirFlagName = "out-dir" + JSONOutDirFlagName = "json-out-dir" + + // If these are intended to be identical to non-deprecated versions, + // consider using the non-deprecated constants directly + DeprecatedOutDirFlagName = OutDirFlagName + DeprecatedJSONOutDirFlagName = JSONOutDirFlagName +)test/integration_test.go (1)
3812-3846: Great refactoring with a test case-driven approach!You've improved the
TestStorePlanFilesJsonRelativePathfunction by using a table-driven test pattern that verifies multiple command formats. This:
- Tests various ways to use the new
--allflag (both as a subcommand and as a flag)- Ensures all command variants produce the same results
- Makes adding future test cases easier
This approach provides excellent coverage for the new CLI patterns while maintaining backward compatibility with existing commands.
Could you add a brief comment explaining the purpose of each test case variant? It would help make the differences between these commands more obvious to future maintainers.
testCases := []struct { args string }{ + // Legacy syntax with run-all subcommand {"run-all plan --terragrunt-non-interactive --log-level trace --working-dir %s --out-dir test --json-out-dir json"}, + // New syntax with --all flag after run {"run --all plan --experiment cli-redesign --non-interactive --log-level trace --working-dir %s --out-dir test --json-out-dir json"}, + // New syntax with --all flag after plan {"run plan --all --experiment cli-redesign --non-interactive --log-level trace --working-dir %s --out-dir test --json-out-dir json"}, + // New syntax with command after -- separator {"run --all --experiment cli-redesign --non-interactive --log-level trace --working-dir %s --out-dir test --json-out-dir json -- plan"}, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cli/commands/aws-provider-patch/command.go(2 hunks)cli/commands/common/graph/action.go(2 hunks)cli/commands/common/graph/command.go(1 hunks)cli/commands/common/runall/action_test.go(1 hunks)cli/commands/common/runall/command.go(1 hunks)cli/commands/graph-dependencies/command.go(2 hunks)cli/commands/graph/command.go(2 hunks)cli/commands/hclfmt/command.go(2 hunks)cli/commands/render-json/command.go(2 hunks)cli/commands/run-all/command.go(2 hunks)cli/commands/run/command.go(5 hunks)cli/commands/run/flags.go(0 hunks)cli/commands/stack/action.go(1 hunks)cli/commands/terragrunt-info/command.go(2 hunks)cli/commands/validate-inputs/command.go(2 hunks)test/integration_graph_test.go(2 hunks)test/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- cli/commands/run/flags.go
✅ Files skipped from review due to trivial changes (2)
- cli/commands/stack/action.go
- cli/commands/common/runall/action_test.go
🔇 Additional comments (54)
cli/commands/graph-dependencies/command.go (3)
5-6: Good addition of runall and graph packagesThese new imports bring in the functionality needed for the
--alland--graphflag implementations, which matches the PR objectives. Well-structured approach to reuse shared code.
17-23: Clean refactoring to prepare for command wrappingBreaking this down into initializing a command variable first, rather than returning directly, makes the code more readable and maintainable.
24-27: Great implementation of command wrapping patternI like how you've applied a consistent wrapping pattern that will provide the
--alland--graphfunctionality across multiple commands. This approach keeps the core command logic intact while layering on additional features.cli/commands/validate-inputs/command.go (3)
6-7: Consistent imports across commandsGood job maintaining consistency with the same imports as other command files, making the codebase more predictable.
39-45: Nicely refactored command initializationBreaking out the command initialization to a separate step before wrapping is clean and consistent with other command implementations.
46-49: Consistent command wrapping implementationFollowing the same pattern as in other commands - first wrapping with runall, then with graph functionality. This consistency makes the codebase easier to understand and maintain.
cli/commands/terragrunt-info/command.go (3)
5-7: Consistent imports across commandsMaintaining the same import pattern across all command files is good practice - it makes the codebase more predictable and easier to follow.
17-23: Clean command initializationBreaking out the command initialization allows for the subsequent wrapping steps in a readable way.
24-27: Consistent command wrapping patternThe same wrapping pattern is applied here as in other commands - this consistency is excellent and ensures that all commands behave similarly with respect to the new flags.
cli/commands/render-json/command.go (3)
5-7: Consistent import approachGood job maintaining the same import structure across all command files, which helps with codebase consistency.
58-65: Clean command initializationThis follows the same pattern as other command files - initializing the command first before applying wrappers.
66-69: Command wrapping implementation follows consistent patternThe wrapping order (runall first, then graph) is maintained consistently with other commands. This consistency ensures predictable behavior across all commands.
cli/commands/aws-provider-patch/command.go (4)
33-34: Nice addition of thegraphandrunallimports.
They look well-organized, and it’s great that you grouped them undercommonfor reusability.
66-72: Command creation logic looks straightforward.
Building the command as a struct gives you flexibility. This approach is consistent with the other commands, so everything remains nice and uniform.
74-75: Check wrap order if the outcome depends on the wrapper sequence.
Wrapping first withrunalland then withgraphmight have consequences on flags or subcommands. If you’re confident in this order, carry on! Otherwise, consider reversing or double-checking.
77-77: Returning the final wrapped command.
This final step is a no-fuss approach, so all good here.cli/commands/hclfmt/command.go (4)
5-6:runallandgraphimports fit well.
It's great that you’re keeping each subcommand consistent with the broader wrapper strategy.
79-85: Command struct is nicely laid out.
Good job keeping the flags in a sorted arrangement. This ensures users see them in a predictable order.
87-88: Same wrap-order consideration.
runallthengraphmight matter for how flags get appended or how the commands chain together. Just verify that’s the intended flow.
90-90: Clean exit.
Returning the wrapped command keeps everything tidy. No issues spotted.test/integration_graph_test.go (10)
89-93: Introducingargsin the test struct is a good call.
It lets you parametrize the commands conveniently without rewriting separate test cases for each scenario. Sweet approach!
95-99:graph applytest variant.
This scenario sets up your CLI usage nicely, verifying that the base case and its additional flags play well together.
101-105:run --graph applytest scenario.
This coverage is great—it ensures your rewriting of the command under an experimental flag does what you expect.
107-111:run apply --graphwith short-form flags.
Having a permutation with slightly rearranged flags is smart to confirm order isn’t messing up the logic.
122-122: Formatting the CLI command string for tests.
Double-check that the%splaceholders always match the number of arguments you pass in. It looks fine, but it’s a classic spot for mistakes if someone changes the usage later.
150-154: New test cases for non-Terraform command execution.
These expansions ensure coverage of multiple argument forms (likegraph render-json). Super helpful for catching corner cases.
158-159: Friendly test naming approach.
In-line naming keeps logs quite readable if something goes south. Good stuff.
161-163: Flexible environment prep.
Parallel tests plus the environment copy is a sleek approach for verifying your test fixture consistency.
167-169: Run command with custom arguments.
Again, verify that your placeholders and arguments align. This is basic but critical for stable tests.
171-175: Verifying output files.
Awesome to see you’re ensuring the “graph” or “rendered” artifacts actually appear where expected. This helps prevent silent test regressions.cli/commands/common/graph/action.go (2)
7-7: Swapping to the newrunallimport.
Consistent usage undercommon/runallpackages helps keep everything tidy.
17-20: Reading and validating the Terragrunt config.
Neat approach. This fails early without leftover side effects if the config is missing or unreadable. A+ error handling.cli/commands/graph/command.go (4)
9-9: Good addition of the common graph packageThis import allows you to leverage functionality from the shared graph package, which supports the implementation of the
--graphflag.
25-27: Nice refactoring of flag managementYou've improved code organization by:
- Using the common graph package's
NewFlagsfunction- Filtering out
GraphRootFlagNameto avoid duplication- Appending run-specific flags
This approach centralizes flag definition and makes the code more maintainable.
32-32: Clean flag handlingUsing the
Sort()method directly on the flags makes the code more concise and readable.
50-50: Appropriate delegation to common packageCalling
graph.Runfrom the common package properly delegates execution responsibility and maintains separation of concerns.cli/commands/common/graph/command.go (3)
12-17: Well-defined flag constantsThe constants are clearly defined and include appropriate naming for both current and deprecated flags. This supports backward compatibility while preparing for future improvements.
19-40: Well-structured flag creation functionThe
NewFlagsfunction:
- Handles prefixes appropriately
- Creates flags with clear usage descriptions
- Properly sets up environment variable support
- Includes handling for deprecated flags with appropriate controls
This is a good example of encapsulating complex flag setup in a reusable function.
42-63: Great implementation of the command wrapper patternThe
WrapCommandfunction implements a clean wrapper approach that:
- Adds graph-specific flags to existing commands
- Conditionally executes graph functionality only when the flag is set
- Preserves the original command action for the non-graph case
- Takes care of sorting flags for consistent help output
This pattern makes the
--graphflag functionality easily reusable across multiple commands.cli/commands/run/command.go (5)
8-9: Good imports for new functionalityThese imports enable the integration of the graph and runall features.
22-51: Improved command structure with feature checksThe restructured command now:
- Uses variable assignment for better readability
- Adds a
Beforehook to check for the CLI redesign experiment flag- Maintains backward compatibility through experiment checks
- Preserves the original action logic
Nice job keeping the experimental feature behind a feature flag.
37-43: Good experimental feature protectionThe
Beforefunction ensures the new CLI redesign is only used when explicitly enabled through experimentation flags. This is a solid approach for rolling out breaking changes safely.
53-56: Elegant command wrapping implementationWrapping the command with both
runallandgraphfunctionality:
- Keeps the base command focused on its core responsibilities
- Adds the
--alland--graphfeatures in a modular way- Follows separation of concerns principles
This is a clean way to extend command functionality.
65-75: Improved variable usage for subcommand creationUsing a variable for subcommand creation before assignment improves clarity and makes debugging easier. Good refactoring.
cli/commands/run-all/command.go (4)
9-9: Good import for common runall functionalityThis import enables reuse of the shared runall implementation, supporting the
--allflag feature.
25-27: Well-structured flag managementThis approach:
- Uses the common
runall.NewFlagsfunction with specific filtering- Appends run-specific flags
- Keeps flag handling consistent with other commands
Good job centralizing flag definitions while customizing for specific command needs.
34-34: Clean flag assignmentUsing the sorted flags directly in the command structure improves readability.
49-49: Proper delegation to common packageCalling
runall.Runfrom the common package maintains separation of concerns and ensures consistent behavior across command implementations.cli/commands/common/runall/command.go (5)
32-33: Great cross-tool compatibilityI like that the usage text explicitly mentions both OpenTofu and Terraform. This makes it clear to users that the functionality works with either tool, which is increasingly important as some users migrate to OpenTofu.
41-41: Flag registration is well-implementedThe use of the
WithDeprecatedNamesfunction to maintain backward compatibility while moving toward the new flag naming convention is a good practice. This ensures users with existing scripts won't experience breakage while encouraging adoption of the new flag format.
57-68: Elegant command wrapping implementationThis implementation nicely separates concerns by using the "action wrapping" pattern. If the
--allflag isn't set, it just passes through to the original action. If it is set, it updates the Terragrunt options and redirects to the stack-wide implementation. This keeps the code clean and maintainable.
70-71: Good flag organization practiceSorting the flags after combining them ensures a consistent order in help output, which improves the user experience. This is a small but thoughtful detail that makes the CLI more professional.
53-74:❓ Verification inconclusive
Verify the Run function dependency
The
WrapCommandfunction calls an externalRunfunction that's not defined in this file. Make sure this function exists and is properly exported from the relevant package. While it's likely that this function is defined elsewhere in the codebase, it's worth verifying to avoid potential runtime errors.
🏁 Script executed:
#!/bin/bash # Search for the Run function that's being called from the WrapCommand function rg -A 1 -B 1 "func Run\(.*\*cli\.Context" --type goLength of output: 53
Double-check the
Runfunction existenceThe
WrapCommandfunction calls an externalRunfunction, but our initial search didn’t return any results. Please verify manually that theRunfunction is indeed defined and properly exported from its package. If it’s defined elsewhere in the codebase, ensure that it’s correctly imported in this file to prevent any runtime errors.test/integration_test.go (1)
17-17: Good addition of the runall importThe new import for
runallpackage aligns with the goal of implementing the--allflag functionality.
| path string | ||
| expectedModules []string | ||
| notExpectedModules []string | ||
| }{ | ||
| { | ||
| args: "graph apply --terragrunt-non-interactive --terragrunt-working-dir %s --terragrunt-graph-root %s", |
There was a problem hiding this comment.
I didn't see it, so I'm going to ask to be sure: Do we have a check to see if a user is using both --all and --graph? What happens in that circumstance?
There was a problem hiding this comment.
This won't work. What to do in this case, return error disallowed to use --all and --graph flags simultaneously?
There was a problem hiding this comment.
Ya. Users have asked for the ability to do this, but we don't support it, so we should throw an error.
There was a problem hiding this comment.
Is this message OK?
Using the `--all` and `--graph` flags simultaneously is not supported.
|
We're missing docs updates here, and a status update on the experiment. We'll either want to do that before we merge or shortly afterwards. |
Description
https://linear.app/gruntwork/issue/TG-604/add-all-to-run-command
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Breaking Changes
Tests