separate service and view concerns #7
Conversation
dhth
commented
Nov 12, 2025
dhth
commented
- add output-format flag
- separate service and view
📝 WalkthroughWalkthroughThis pull request refactors the module comparison flow to return structured results instead of writing output directly. Services now provide Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (7)
internal/services/__snapshots__/TestShowModuleComparison_all_modules_in_sync_1.snapis excluded by!**/*.snapinternal/services/__snapshots__/compare_modules_test.snapis excluded by!**/*.snapinternal/view/__snapshots__/stdout_test.snapis excluded by!**/*.snaptests/cli/__snapshots__/TestCompareModulesCmd_fails_for_invalid_output_format_flag_1.snapis excluded by!**/*.snaptests/cli/__snapshots__/TestCompareModulesCmd_help_flag_works_1.snapis excluded by!**/*.snaptests/cli/__snapshots__/TestCompareModulesCmd_ignoring_missing_modules_works_1.snapis excluded by!**/*.snaptests/cli/__snapshots__/TestCompareModulesCmd_works_for_correct_config_1.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
internal/cmd/compare_modules.go(4 hunks)internal/domain/config.go(1 hunks)internal/domain/types.go(1 hunks)internal/services/compare_modules.go(5 hunks)internal/services/compare_modules_test.go(3 hunks)internal/services/testdata/environments/qa/main.tf(1 hunks)internal/view/stdout.go(1 hunks)internal/view/stdout_test.go(1 hunks)main.go(1 hunks)tests/cli/compare_modules_test.go(2 hunks)tests/cli/testdata/environments/qa/main.tf(1 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
internal/services/testdata/environments/qa/main.tf
[medium] 19-23: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
tests/cli/testdata/environments/qa/main.tf
[medium] 19-23: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
🔇 Additional comments (13)
internal/services/testdata/environments/qa/main.tf (1)
19-23: LGTM!The new module_e addition is consistent with the existing module structure and follows the same patterns used for module_a, module_b, and module_c.
tests/cli/testdata/environments/qa/main.tf (1)
19-23: LGTM!The module_e addition is consistent with the existing modules and maintains parity with the internal/services testdata.
internal/domain/config.go (2)
55-60: LGTM!The OutputFormat type definition and constants are well-structured and follow Go idioms for enum-like types.
73-75: LGTM!The function correctly returns all valid output format values.
main.go (1)
17-17: LGTM!The error reference update correctly reflects the architectural change where error ownership moved from the services layer to the command layer.
tests/cli/compare_modules_test.go (2)
54-69: LGTM!The test correctly exercises the new
--ignore-missing-modulesflag and follows the established testing patterns with snapshot validation.
91-106: LGTM!The test correctly validates the error handling for an invalid output format value. The snapshot-based validation ensures the error message remains consistent.
internal/view/stdout_test.go (1)
13-202: LGTM!The test cases provide comprehensive coverage of different scenarios for the RenderStdout function, including in-sync modules, out-of-sync modules, missing modules with different handling, and edge cases. The test structure is consistent and clear.
internal/view/stdout.go (1)
11-42: LGTM!The RenderStdout function is well-implemented with clean structure, appropriate error handling, and proper use of tabwriter for formatted output. The handling of missing values with "-" and the use of Status.Symbol() for visual indicators are good design choices.
internal/cmd/compare_modules.go (4)
14-17: LGTM!The error declarations are appropriate, with correct capitalization for exported vs internal errors.
22-23: LGTM!The new flag variables are appropriately typed and named.
104-126: LGTM with a note on the HTML placeholder.The refactoring to a result-based flow is well-structured. The stdout rendering path correctly uses the view layer, renders the output, and then checks module synchronization status. The HTML output path is currently a placeholder, which is acceptable for this PR.
138-152: LGTM!The new flag definitions are well-structured with appropriate types, defaults, and clear help text. The short flag choices (
-iand-o) are intuitive and don't conflict with existing flags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/domain/config.go (2)
55-60: Consider adding documentation comments.The new public
OutputFormattype and its constants lack documentation comments. Adding these would improve discoverability and align with Go conventions.Example:
+// OutputFormat represents the format for rendering output. type OutputFormat uint8 const ( + // StdoutOutput renders output to standard output in plain text format. StdoutOutput OutputFormat = iota + // HtmlOutput renders output in HTML format. HtmlOutput )
73-75: Consider documentation and potential for drift.Two optional improvements:
- Add a documentation comment explaining the function's purpose.
- The hardcoded values could drift from
ParseOutputFormatif new formats are added. Consider either documenting this relationship or refactoring to derive values from a shared source.Example documentation:
+// GetOutputFormatValues returns the list of valid output format strings. func GetOutputFormatValues() []string { return []string{"stdout", "html"} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
internal/view/__snapshots__/stdout_test.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
internal/cmd/compare_modules.go(4 hunks)internal/domain/config.go(1 hunks)internal/view/stdout_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/view/stdout_test.go
- internal/cmd/compare_modules.go
🔇 Additional comments (1)
internal/domain/config.go (1)
62-71: Critical issue resolved.The mapping for the HTML output format has been corrected. Line 67 now properly returns
HtmlOutput, trueinstead of the previously incorrectStdoutOutput, true.
