Refactor OpenTofu CLI commands and tests #5
Conversation
…n and update related test cases for improved clarity and functionality.
…tting - Removed existing tests for tofu validation and added new implementation in cmd/tofu_validate. - Created new command for formatting OpenTofu files in cmd/tofu_fmt. - Added tests for both tofu validation and formatting functionalities. - Implemented helper functions for running commands and finding directories with .tf files. - Enhanced output handling with colored status messages. - Updated internal outputs to include new test cases for colorization and emoji text.
…o 'tofu_fmt' and 'tofu-validate' to 'tofu_validate' for consistency.
… fmt' and 'tofu validate' for consistency; improve argument handling in validation command; enhance output formatting for better readability.
|
Warning Rate limit exceeded@brettcurtis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
""" WalkthroughThis change reorganizes and refactors the OpenTofu formatter and validator CLI tools, splitting their logic into new internal packages ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Complexity label: Complex
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
…d workflow resilience
…ng in tofu fmt, consolidate test utility functions, and improve test structure for OpenTofu validation.
…xit function and return error for validation failure
…irectory management
…ndling in directory restoration, and add logging for installation checks in tests
…kage, update references, and enhance tests for emoji color text functionality
… tofuvalidate for consistency and clarity
…sure job name is clearly defined
…e hooks with usage examples
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the OpenTofu CLI commands by migrating from the deprecated cmd/tofu-* structure to a new cmd/tofu* naming convention and implements comprehensive validation and formatting functionality. The changes include reorganizing code into reusable internal packages, enhancing test coverage with proper utility functions, and improving output consistency with colored status messages.
- Migrates
cmd/tofu-validate→cmd/tofuvalidateandcmd/tofu-fmt→cmd/tofufmtwith consistent naming - Introduces shared testutil package with OpenTofu installation checking and directory management utilities
- Refactors output package from
outputstooutputwith improved emoji constants and colored text functions
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/tofuvalidate/ |
New package implementing OpenTofu validation logic with comprehensive test coverage |
internal/tofufmt/ |
New package implementing OpenTofu formatting logic with multi-file and nested directory tests |
internal/testutil/ |
Shared utility package for OpenTofu installation checks and test directory management |
internal/output/ |
Refactored from outputs with improved emoji constants and color text functions |
cmd/tofuvalidate/ |
New CLI command replacing cmd/tofu-validate with enhanced error handling and dependency injection |
cmd/tofufmt/ |
New CLI command replacing cmd/tofu-fmt with improved formatting workflow |
.pre-commit-hooks.yaml |
Updated hook entries to use new command names (tofufmt, tofuvalidate) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
cmd/tofufmt/main.go (1)
24-65: Well-structured CLI logic with one improvement opportunity.The function demonstrates excellent separation of concerns and user-friendly messaging. However, consider distinguishing between different types of errors from
runTofuFmt- not all errors indicate unformatted files.Consider checking the error type or exit code to differentiate between "unformatted files" errors and other failures:
outputStr, err := runTofuFmt(wd, extraArgs) fmt.Println() if err != nil { - fmt.Println(output.EmojiColorText(output.Warning, "Found unformatted OpenTofu files:", output.Yellow)) - fmt.Println(outputStr) - printStatus(output.Running, "Formatting files with tofu fmt...") - fmtErr := formatFiles(wd, extraArgs) + // Check if this is specifically an "unformatted files" error (exit code 3) + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 3 { + fmt.Println(output.EmojiColorText(output.Warning, "Found unformatted OpenTofu files:", output.Yellow)) + fmt.Println(outputStr) + printStatus(output.Running, "Formatting files with tofu fmt...") + fmtErr := formatFiles(wd, extraArgs) + // ... rest of formatting logic + } else { + fmt.Println(output.EmojiColorText(output.Error, "Error running tofu fmt:", output.Red)) + fmt.Println(err) + return err + }internal/tofuvalidate/tofuvalidate_test.go (1)
66-74: Misleading comment about test flowThe comment suggests the test continues after init failure, but the code shows it fails the test if init succeeds. Consider clarifying:
- // Run tofu init first (expected to fail, but test continues to run validation and checks for an error there; test does not pass solely on init failure) + // Run tofu init first (expected to fail for invalid syntax) initCmd := []string{"init", "-input=false", "--backend=false"} cmd := exec.Command("tofu", initCmd...) cmd.Dir = tempDir out, err := cmd.CombinedOutput() if err == nil { t.Fatalf("Expected tofu init to fail due to invalid config, but it succeeded. Output: %s", out) } - // If tofu init fails as expected, test passes + // Continue to test validation even though init failedcmd/tofuvalidate/main.go (1)
159-176: Consider simplifying output formatting logicThe
lastNonEmptycalculation and newline logic could be simplified. The current check on line 173 might not work as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/go-test.yml(1 hunks).pre-commit-hooks.yaml(1 hunks)README.md(1 hunks)cmd/tofu-fmt/main.go(0 hunks)cmd/tofu-fmt/main_test.go(0 hunks)cmd/tofu-validate/main.go(0 hunks)cmd/tofu-validate/main_test.go(0 hunks)cmd/tofufmt/main.go(1 hunks)cmd/tofufmt/main_test.go(1 hunks)cmd/tofuvalidate/main.go(1 hunks)cmd/tofuvalidate/main_test.go(1 hunks)internal/output/output.go(2 hunks)internal/output/output_test.go(1 hunks)internal/outputs/main_test.go(0 hunks)internal/testutil/testutil.go(1 hunks)internal/testutil/testutil_test.go(1 hunks)internal/tofufmt/tofufmt.go(1 hunks)internal/tofufmt/tofufmt_test.go(1 hunks)internal/tofuvalidate/tofuvalidate.go(1 hunks)internal/tofuvalidate/tofuvalidate_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- internal/outputs/main_test.go
- cmd/tofu-fmt/main.go
- cmd/tofu-fmt/main_test.go
- cmd/tofu-validate/main.go
- cmd/tofu-validate/main_test.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: brettcurtis
PR: osinfra-io/pre-commit-hooks#1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-06-22T23:33:42.814Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
Learnt from: brettcurtis
PR: osinfra-io/pre-commit-hooks#1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: main.tf:0-0
Timestamp: 2024-07-14T11:41:16.733Z
Learning: The user `brettcurtis` fixed the issue of using a specific version tag for the `kubernetes_istio` module source reference in commit `9ce3c9a`.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: main.tf:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: The user `brettcurtis` fixed the issue of using a specific version tag for the `kubernetes_istio` module source reference in commit `9ce3c9a`.
Learnt from: brettcurtis
PR: osinfra-io/terraform-google-cloud-dns#23
File: .pre-commit-config.yaml:31-37
Timestamp: 2024-10-12T15:07:34.851Z
Learning: The combination of `verbose: true` and `--quiet` in the Checkov pre-commit hook configuration is used to provide details on failed tests while suppressing output for passing tests, as explained by brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/terraform-google-cloud-dns#23
File: .pre-commit-config.yaml:31-37
Timestamp: 2024-06-09T12:07:49.811Z
Learning: The combination of `verbose: true` and `--quiet` in the Checkov pre-commit hook configuration is used to provide details on failed tests while suppressing output for passing tests, as explained by brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/datadog-organization-management#8
File: opa/labels_test.rego:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: When the user brettcurtis requests to create a new issue for assertions, ensure to summarize the context and action items clearly, and assign the issue to brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/datadog-organization-management#8
File: opa/labels_test.rego:0-0
Timestamp: 2024-09-02T15:26:52.062Z
Learning: When the user brettcurtis requests to create a new issue for assertions, ensure to summarize the context and action items clearly, and assign the issue to brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: regional/main.tf:0-0
Timestamp: 2024-07-14T11:40:35.132Z
Learning: When suggesting changes to Terraform module references, ensure to use specific version tags for stability and reproducibility. The user, brettcurtis, prefers using specific version tags for Terraform module references.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: regional/main.tf:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: When suggesting changes to Terraform module references, ensure to use specific version tags for stability and reproducibility. The user, brettcurtis, prefers using specific version tags for Terraform module references.
Learnt from: brettcurtis
PR: osinfra-io/terraform-kubernetes-cert-manager#1
File: .terraform-docs.yml:4-5
Timestamp: 2024-09-29T11:04:17.455Z
Learning: brettcurtis prefers to revisit the `terraform-docs` settings globally at a later time rather than making changes immediately.
.pre-commit-hooks.yaml (1)
Learnt from: brettcurtis
PR: osinfra-io/github-organization-management#169
File: .pre-commit-config.yaml:6-6
Timestamp: 2024-10-19T13:56:36.246Z
Learning: When pre-commit hooks are updated in .pre-commit-config.yaml, it's not necessary to suggest running them manually on the entire codebase because they will automatically run before each commit.
internal/output/output_test.go (2)
Learnt from: brettcurtis
PR: #1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
Learnt from: brettcurtis
PR: #1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-06-22T23:33:42.814Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
internal/testutil/testutil.go (1)
Learnt from: brettcurtis
PR: osinfra-io/istio-test#1
File: main_test.go:4-8
Timestamp: 2024-07-29T01:17:15.774Z
Learning: The ioutil package is deprecated and its functions have been moved to io and os packages in Go 1.16. For example, ioutil.NopCloser is now io.NopCloser.
README.md (5)
Learnt from: brettcurtis
PR: osinfra-io/github-organization-management#0
File: :0-0
Timestamp: 2024-07-29T01:17:15.774Z
Learning: The entire Terraform documentation section in global/README.md is auto-generated by Terraform-docs. Manual changes to this section should be avoided as they will be overwritten.
Learnt from: brettcurtis
PR: osinfra-io/opentofu-google-kubernetes-engine#41
File: regional/onboarding/README.md:35-38
Timestamp: 2024-07-29T01:17:15.774Z
Learning: Documentation in Terraform projects, such as regional/onboarding/README.md, is often automatically generated by tools like terraform-docs based on the variables, outputs, resources, modules, and providers. Future suggestions related to documentation should consider the source of the documentation, such as variable descriptions or other elements that are automatically generated.
Learnt from: brettcurtis
PR: osinfra-io/github-organization-management#147
File: global/infra/README.md:12-12
Timestamp: 2024-07-29T01:17:15.774Z
Learning: The Terraform documentation section in global/infra/README.md is auto-generated by Terraform-docs. Similar issues related to provider version updates or naming conventions should be ignored in future reviews.
Learnt from: brettcurtis
PR: osinfra-io/github-organization-management#169
File: .pre-commit-config.yaml:6-6
Timestamp: 2024-10-19T13:56:36.246Z
Learning: When pre-commit hooks are updated in .pre-commit-config.yaml, it's not necessary to suggest running them manually on the entire codebase because they will automatically run before each commit.
Learnt from: brettcurtis
PR: osinfra-io/github-organization-management#0
File: :0-0
Timestamp: 2024-07-29T01:17:15.774Z
Learning: Feedback should not be provided on sections of README.md files that are auto-generated by Terraform-docs, as indicated by user brettcurtis in PR osinfra-io/github-organization-management#154.
internal/tofufmt/tofufmt.go (2)
Learnt from: brettcurtis
PR: #1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-06-22T23:33:42.814Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
Learnt from: brettcurtis
PR: #1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
internal/tofuvalidate/tofuvalidate_test.go (2)
Learnt from: brettcurtis
PR: osinfra-io/opentofu-google-kubernetes-engine#41
File: test/fixtures/gke_fleet_host/regional_mci/main.tf:25-32
Timestamp: 2024-07-29T01:17:15.774Z
Learning: Test fixtures for Kitchen-Terraform, such as those in the terraform-google-kubernetes-engine project, are designed with hardcoded values to ensure consistency and controlled conditions in testing environments. These fixtures are not intended to support flexibility across different environments, as clarified by user brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/opentofu-core-helpers#5
File: shared/helpers.tf:71-75
Timestamp: 2024-11-13T00:35:26.219Z
Learning: In shared/helpers.tf, the workspace variable is used in tests with various workspace names, including real ones. Therefore, adding validation to restrict its value is not appropriate.
cmd/tofuvalidate/main_test.go (3)
Learnt from: brettcurtis
PR: osinfra-io/opentofu-core-helpers#5
File: shared/helpers.tf:71-75
Timestamp: 2024-11-13T00:35:26.219Z
Learning: In shared/helpers.tf, the workspace variable is used in tests with various workspace names, including real ones. Therefore, adding validation to restrict its value is not appropriate.
Learnt from: brettcurtis
PR: osinfra-io/opentofu-google-kubernetes-engine#41
File: test/fixtures/gke_fleet_host/regional_mci/main.tf:25-32
Timestamp: 2024-07-29T01:17:15.774Z
Learning: Test fixtures for Kitchen-Terraform, such as those in the terraform-google-kubernetes-engine project, are designed with hardcoded values to ensure consistency and controlled conditions in testing environments. These fixtures are not intended to support flexibility across different environments, as clarified by user brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/opentofu-google-kubernetes-engine#41
File: test/fixtures/gke_fleet_member/global/variables.tf:9-12
Timestamp: 2024-07-29T01:17:15.774Z
Learning: The test/fixtures path contains test fixtures for Kitchen-Terraform, which are designed for specific testing scenarios and not intended to support flexibility across different environments. Hardcoded values in these fixtures are intentional to ensure consistency.
🧬 Code Graph Analysis (4)
cmd/tofufmt/main.go (5)
internal/tofufmt/tofufmt.go (3)
RunTofuFmt(14-20)FormatFiles(23-28)CheckOpenTofuInstalled(10-10)internal/tofuvalidate/tofuvalidate.go (1)
CheckOpenTofuInstalled(10-10)internal/output/output.go (8)
Running(27-27)EmojiColorText(33-35)Warning(25-25)Yellow(12-12)Error(23-23)Red(10-10)ThumbsUp(29-29)Green(11-11)cmd/tofu-fmt/main.go (2)
main(61-103)printStatus(106-108)cmd/tofu-fmt/main_test.go (1)
TestOpenTofuFormat(11-95)
internal/testutil/testutil_test.go (1)
internal/testutil/testutil.go (5)
SkipIfTofuNotInstalled(16-20)CheckOpenTofuInstalled(10-13)CreateTempDir(23-30)RestoreWorkingDir(33-46)RunCmd(49-53)
internal/tofuvalidate/tofuvalidate.go (3)
internal/tofufmt/tofufmt.go (1)
CheckOpenTofuInstalled(10-10)cmd/tofu-validate/main.go (2)
main(19-88)checkOpenTofuInstalled(14-17)cmd/tofu-validate/main_test.go (2)
TestValidOpenTofuConfig(28-85)TestCheckOpenTofuInstalled(11-25)
internal/tofufmt/tofufmt.go (3)
internal/tofuvalidate/tofuvalidate.go (1)
CheckOpenTofuInstalled(10-10)cmd/tofu-fmt/main.go (1)
main(61-103)cmd/tofu-fmt/main_test.go (1)
TestOpenTofuFormat(11-95)
🔇 Additional comments (42)
internal/output/output.go (2)
1-1: Good package name standardization.The change from
outputstooutputfollows Go naming conventions where package names are typically singular nouns.
22-29: Excellent documentation improvement.The descriptive comments for each emoji constant significantly improve code readability and make the purpose of each constant clear. This aligns well with the modular approach and maintainability goals.
.pre-commit-hooks.yaml (2)
3-3: LGTM: Consistent command entry point updates.The changes from hyphenated commands (
tofu-fmt,tofu-validate) to single-word commands (tofufmt,tofuvalidate) align with the new CLI tool implementations and maintain consistency across both hooks.Also applies to: 12-12
7-7: Good naming simplification.The updated hook names ("tofu fmt", "tofu validate") are more concise while remaining descriptive and user-friendly.
Also applies to: 18-18
internal/tofuvalidate/tofuvalidate.go (3)
1-7: Excellent modular design.The new
tofuvalidatepackage properly extracts validation logic into a reusable component. The imports are appropriate and the package structure follows Go conventions.
9-10: Good code reuse through delegation.Delegating
CheckOpenTofuInstalledto the sharedtestutilpackage promotes code reuse and maintains consistency across the codebase. This aligns with your preference for modular utility functions.
12-20: Well-implemented validation function.The
RunTofuValidatefunction is correctly implemented:
- Proper argument handling by appending extra args to the base
validatecommand- Correct working directory setting with
cmd.Dir- Appropriate use of
CombinedOutput()to capture both stdout and stderr- Clean return of output and error for proper error handling
.github/workflows/go-test.yml (4)
1-11: Well-structured workflow configuration.The workflow setup is excellent with appropriate triggers for PR testing and proper job configuration using ubuntu-latest.
20-24: Good test execution and coverage setup.The test execution properly captures output and generates coverage reports. Using
teeto save output while displaying it is a good practice.
26-39: Excellent summary generation with proper error handling.The GitHub Actions summary generation is well-implemented:
- Properly reads test results and coverage files
- Creates formatted summary with headings and code blocks
- Uses
continue-on-error: trueto prevent workflow failure if summary generation fails- Good use of
actions/github-script@v7for the implementation
15-18: No action needed – Go 1.24.5 is available.Confirmed that Go 1.24.5 is the latest stable release (released July 8, 2025) and is valid for use in your workflow.
README.md (3)
1-5: Excellent documentation foundation.The introduction clearly establishes the repository's purpose and provides context about pre-commit hooks. The structure is well-organized and professional.
6-19: Clear and comprehensive hook descriptions.The documentation for both
tofu-fmtandtofu-validatehooks is excellent:
- Clear purpose statements for each hook
- Detailed explanations of what files are processed
- Important notes about
.terraform/directory exclusions- Good balance of technical detail and accessibility
20-54: Practical and well-formatted usage examples.The usage section provides excellent practical guidance:
- Clear YAML examples that users can copy and modify
- Proper placeholder for version/SHA (
<release-or-commit-sha>)- Good documentation of optional arguments
- Consistent formatting and structure across both hook examples
- Helpful reference to the
.pre-commit-hooks.yamlfileThis documentation will significantly improve the user experience for adopting these hooks.
internal/tofufmt/tofufmt.go (4)
1-7: Clean package structure and imports.The package declaration and imports are well-organized and follow Go conventions. The dependency on
internal/testutilaligns with the modular approach.
9-10: Good use of delegation pattern.The delegation to
testutil.CheckOpenTofuInstalledpromotes code reuse and maintains consistency across internal packages, aligning with your preference for modular utility functions.
12-20: Well-implemented formatting check function.The function correctly constructs the
tofu fmtcommand with appropriate flags (-check,-recursive,--diff) and handles directory context properly. UsingCombinedOutput()is the right choice for capturing diff output.
22-28: Efficient file formatting implementation.The function appropriately uses
cmd.Run()for formatting operations where output capture isn't needed, making it more efficient than the check function. The design nicely complementsRunTofuFmt.internal/output/output_test.go (3)
7-25: Comprehensive emoji and color text testing.The table-driven test approach effectively covers the main emoji/color combinations used throughout the CLI tools. The test cases align well with the actual usage patterns in the codebase.
27-43: Thorough Reset color constant testing.The test properly validates both the ANSI escape sequence value and the practical usage of the Reset constant in the Colorize function, ensuring terminal color reset functionality works correctly.
45-61: Complete color testing coverage.The table-driven test effectively validates all color constants with correct ANSI escape sequences. The test ensures proper text colorization with reset sequences for all supported colors.
internal/testutil/testutil_test.go (4)
9-14: Appropriate external tool dependency testing.The test correctly uses
SkipIfTofuNotInstalledto avoid false failures when the external tool isn't available, while still validating the function behavior when the tool is present.
16-26: Well-structured temporary directory testing.The test properly uses the cleanup function and verifies both the existence and directory nature of the created temporary directory. Good use of defer for resource cleanup.
28-67: Comprehensive working directory restoration testing.The test thoroughly handles symbolic link resolution using
filepath.EvalSymlinks, which is important for cross-platform compatibility. It properly verifies both the directory change and restoration operations.
69-77: Simple and effective command execution testing.The test uses a universally available command (
echo) to verify both successful execution and accurate output capture, including proper handling of newline characters.internal/testutil/testutil.go (5)
9-13: Correct external tool availability check.Using
exec.LookPathis the standard and reliable approach for checking if an external binary is available in the system PATH.
15-20: Well-designed test skipping utility.The function provides a clean way to skip tests when external dependencies aren't available, preventing false failures while maintaining clear communication about why tests were skipped.
22-30: Excellent temporary directory utility design.The function follows Go testing best practices by using
os.MkdirTempand returning a cleanup function for proper resource management. The error handling witht.Fatalfis appropriate for test setup failures.
32-46: Thoughtful working directory management.The function uses appropriate error handling strategies -
t.Fatalffor critical setup failures andt.Errorffor restoration issues. This design provides robust directory management for testing scenarios.
48-53: Clean command execution utility.The function provides a simple and flexible interface for running external commands with proper output capture using
CombinedOutput(). The variadic args design makes it adaptable to various command patterns.cmd/tofufmt/main.go (2)
12-22: Excellent dependency injection design.The main function demonstrates good separation of concerns by injecting dependencies into
RunTofuFmtCLI, making the core logic testable while keeping the entry point simple and focused.
67-70: Clean status message utility.The function provides consistent status message formatting using the output package's emoji and color functionality, creating a cohesive user experience.
internal/tofufmt/tofufmt_test.go (1)
82-111: Well-structured test implementationThe test properly validates the formatting behavior for unformatted files with clear setup, execution, and verification phases.
internal/tofuvalidate/tofuvalidate_test.go (1)
22-51: Well-implemented validation testThe test properly validates a valid Terraform configuration with appropriate setup and error handling.
cmd/tofuvalidate/main.go (2)
14-35: Clean dependency injection patternGood use of dependency injection for testability. The filtering of arguments to only pass flags is appropriate for tofu init/validate commands.
131-156: Well-implemented directory traversalThe recursive directory traversal correctly identifies Terraform directories while appropriately skipping hidden and .terraform directories.
cmd/tofufmt/main_test.go (3)
16-62: Comprehensive test coverage with good patternsExcellent use of table-driven tests covering all branches. Note that modifying global state (lines 50-52) could cause issues with parallel test execution, but the defer ensures proper cleanup.
113-123: Good acknowledgment of testing limitationsThe comment acknowledging the inability to test when tofu is not installed shows good testing awareness.
125-178: Well-implemented integration testThe test properly validates the formatting functionality with real file operations and appropriate setup/cleanup.
cmd/tofuvalidate/main_test.go (3)
14-116: Excellent test coverage for utility functionsThe tests thoroughly cover error cases, hidden directories, and edge cases for all utility functions.
118-234: Excellent branch coverage with comprehensive scenariosThe table-driven tests and additional branch-specific tests provide thorough coverage of all code paths in the CLI logic.
244-311: Well-implemented integration testsThe integration tests properly validate the actual tofu commands with appropriate setup and cleanup.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…r on false result
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cmd/tofuvalidate/main_test.go (2)
25-30: Use testutil.CreateTempDir for consistency.Replace the manual temp directory creation with
testutil.CreateTempDirto maintain consistency with other test functions in this file and reduce code duplication.- tempDir, err := os.MkdirTemp("", "finddirs_test") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) + tempDir, cleanup := testutil.CreateTempDir(t, "finddirs_test") + defer cleanup()
75-80: Use testutil.CreateTempDir for consistency.Replace the manual temp directory creation with
testutil.CreateTempDir:- tempDir, err2 := os.MkdirTemp("", "walkdirs_test") - if err2 != nil { - t.Fatalf("Failed to create temp dir: %v", err2) - } - defer os.RemoveAll(tempDir) + tempDir, cleanup := testutil.CreateTempDir(t, "walkdirs_test") + defer cleanup()
🧹 Nitpick comments (1)
cmd/tofuvalidate/main_test.go (1)
14-22: Consider adding a test case for successful command execution.The test only covers the error case. Consider adding a positive test case to ensure the function works correctly with valid commands.
Add a test case for successful execution:
func Test_runCmdInDir(t *testing.T) { + // Test error case out, err := runCmdInDir(".", []string{"nonexistentcmd"}) if err == nil { t.Error("Expected error for nonexistent command") } if out == "" { t.Log("Output is empty as expected for nonexistent command") } + + // Test success case + out, err = runCmdInDir(".", []string{"echo", "test"}) + if err != nil { + t.Errorf("Unexpected error for echo command: %v", err) + } + if !strings.Contains(out, "test") { + t.Errorf("Expected output to contain 'test', got: %s", out) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/tofuvalidate/main.go(1 hunks)cmd/tofuvalidate/main_test.go(1 hunks)internal/tofufmt/tofufmt_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tofufmt/tofufmt_test.go
- cmd/tofuvalidate/main.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: brettcurtis
PR: osinfra-io/pre-commit-hooks#1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-06-22T23:33:42.814Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
Learnt from: brettcurtis
PR: osinfra-io/pre-commit-hooks#1
File: hooks/helpers/outputs.go:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: User brettcurtis prefers a more modular approach for utility functions, as demonstrated in the refactoring of text colorization functions in outputs.go to reduce code duplication and enhance maintainability.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: main.tf:0-0
Timestamp: 2024-07-14T11:41:16.733Z
Learning: The user `brettcurtis` fixed the issue of using a specific version tag for the `kubernetes_istio` module source reference in commit `9ce3c9a`.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: main.tf:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: The user `brettcurtis` fixed the issue of using a specific version tag for the `kubernetes_istio` module source reference in commit `9ce3c9a`.
Learnt from: brettcurtis
PR: osinfra-io/terraform-google-cloud-dns#23
File: .pre-commit-config.yaml:31-37
Timestamp: 2024-06-09T12:07:49.811Z
Learning: The combination of `verbose: true` and `--quiet` in the Checkov pre-commit hook configuration is used to provide details on failed tests while suppressing output for passing tests, as explained by brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/terraform-google-cloud-dns#23
File: .pre-commit-config.yaml:31-37
Timestamp: 2024-10-12T15:07:34.851Z
Learning: The combination of `verbose: true` and `--quiet` in the Checkov pre-commit hook configuration is used to provide details on failed tests while suppressing output for passing tests, as explained by brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/datadog-organization-management#8
File: opa/labels_test.rego:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: When the user brettcurtis requests to create a new issue for assertions, ensure to summarize the context and action items clearly, and assign the issue to brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/datadog-organization-management#8
File: opa/labels_test.rego:0-0
Timestamp: 2024-09-02T15:26:52.062Z
Learning: When the user brettcurtis requests to create a new issue for assertions, ensure to summarize the context and action items clearly, and assign the issue to brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: regional/main.tf:0-0
Timestamp: 2024-10-08T15:39:14.063Z
Learning: When suggesting changes to Terraform module references, ensure to use specific version tags for stability and reproducibility. The user, brettcurtis, prefers using specific version tags for Terraform module references.
Learnt from: brettcurtis
PR: osinfra-io/google-cloud-kubernetes#55
File: regional/main.tf:0-0
Timestamp: 2024-07-14T11:40:35.132Z
Learning: When suggesting changes to Terraform module references, ensure to use specific version tags for stability and reproducibility. The user, brettcurtis, prefers using specific version tags for Terraform module references.
Learnt from: brettcurtis
PR: osinfra-io/terraform-kubernetes-cert-manager#1
File: .terraform-docs.yml:4-5
Timestamp: 2024-09-29T11:04:17.455Z
Learning: brettcurtis prefers to revisit the `terraform-docs` settings globally at a later time rather than making changes immediately.
cmd/tofuvalidate/main_test.go (3)
Learnt from: brettcurtis
PR: osinfra-io/opentofu-core-helpers#5
File: shared/helpers.tf:71-75
Timestamp: 2024-11-13T00:35:26.219Z
Learning: In shared/helpers.tf, the workspace variable is used in tests with various workspace names, including real ones. Therefore, adding validation to restrict its value is not appropriate.
Learnt from: brettcurtis
PR: osinfra-io/opentofu-google-kubernetes-engine#41
File: test/fixtures/gke_fleet_host/regional_mci/main.tf:25-32
Timestamp: 2024-07-29T01:17:15.774Z
Learning: Test fixtures for Kitchen-Terraform, such as those in the terraform-google-kubernetes-engine project, are designed with hardcoded values to ensure consistency and controlled conditions in testing environments. These fixtures are not intended to support flexibility across different environments, as clarified by user brettcurtis.
Learnt from: brettcurtis
PR: osinfra-io/opentofu-google-kubernetes-engine#41
File: test/fixtures/gke_fleet_member/global/variables.tf:9-12
Timestamp: 2024-07-29T01:17:15.774Z
Learning: The test/fixtures path contains test fixtures for Kitchen-Terraform, which are designed for specific testing scenarios and not intended to support flexibility across different environments. Hardcoded values in these fixtures are intentional to ensure consistency.
🔇 Additional comments (3)
cmd/tofuvalidate/main_test.go (3)
118-234: Excellent test structure and coverage!The table-driven approach with dependency injection provides comprehensive coverage of all branches and error scenarios. The additional subtests for relPath rewriting and multi-error handling are particularly thorough.
236-242: Good integration test implementation.Proper use of
testutil.SkipIfTofuNotInstalledto handle environments without tofu installed.
244-275: Well-implemented integration test with proper testutil usage.Excellent use of
testutilpackage functions for temp directory creation and working directory restoration. The test properly validates a valid OpenTofu configuration.
… tofu binary presence
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Enhance the OpenTofu CLI by implementing validation and formatting commands, improving test clarity, and ensuring consistent naming in pre-commit hooks. This update also includes better output handling with colored status messages.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores