add pager to atmos describe affected command #1255
Conversation
…mand' of https://github.com/cloudposse/atmos into feature/dev-3131-add-pager-to-atmos-describe-config-command
…mand' of https://github.com/cloudposse/atmos into feature/dev-3131-add-pager-to-atmos-describe-config-command
…mand' of https://github.com/cloudposse/atmos into feature/dev-3131-add-pager-to-atmos-describe-config-command
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…mand' of https://github.com/cloudposse/atmos into feature/dev-3131-add-pager-to-atmos-describe-config-command
cbc8b47 to
2071474
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/describe_affected.go(2 hunks)internal/exec/describe_affected.go(5 hunks)internal/exec/describe_affected_test.go(1 hunks)internal/exec/file_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/file_utils.go
🧰 Additional context used
🧠 Learnings (1)
cmd/describe_affected.go (1)
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected.go:122-123
Timestamp: 2025-05-22T19:58:32.988Z
Learning: The "pager" flag is defined as a PersistentFlag at the describe command level in cmd/describe.go, making it available to all subcommands including describeAffectedCmd without needing to redeclare it.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (11)
internal/exec/describe_affected_test.go (2)
14-63: Solid refactoring to interface-based testing approach.The new test structure is much cleaner and follows good testing practices. Using dependency injection with stubbed functions allows for better isolation and more predictable test behavior. The TTY support testing with mocks is particularly well done.
Consider adding a few more test scenarios:
- Test error cases (when stubbed functions return errors)
- Test the flag validation logic more thoroughly
- Test the automatic enabling of dependents/settings when upload is true
67-68: Good cleanup with t.Setenv.Using
t.Setenvinstead of manualos.Setenv/os.Unsetenvcalls is the right approach - it provides automatic cleanup and prevents test pollution.cmd/describe_affected.go (4)
30-43: Clean higher-order function design for dependency injection.The
getRunnableDescribeAffectedCmdfunction nicely separates concerns by accepting functions as parameters, making the code more testable and modular. This approach allows for easy mocking in tests and follows the dependency injection pattern well.
104-105: Comprehensive flag conflict validation.Good validation to prevent conflicting usage patterns. The
ErrRepoPathConflicterror clearly communicates the mutual exclusivity between--repo-pathand the other target reference flags.
138-147: Complete type handling for flag parsing.The switch statement now properly handles all flag types including
*[]stringfor slice flags. This prevents the panic that would have occurred with theskipflag. Good fix based on the past review feedback.
151-154: Smart automatic flag enabling for upload functionality.When upload is enabled, automatically enabling
IncludeDependentsandIncludeSettingsmakes sense - you'd want complete information when uploading affected components to an external system.internal/exec/describe_affected.go (5)
38-41: Clean interface design with mock generation.The
DescribeAffectedExecinterface is well-designed with a single responsibility. Including the//go:generatedirective for mock generation shows good testing forethought.
43-95: Excellent dependency injection structure.The
describeAffectedExecstruct uses dependency injection beautifully, making all external dependencies explicit and testable. This approach allows for easy mocking in tests and follows the SOLID principles well.
112-171: Well-structured execution dispatch logic.The
Executemethod cleanly dispatches to different execution strategies based on the input flags. The switch statement covers all the execution paths (repo path, clone target ref, and default checkout), and error handling is consistent throughout.
239-257: Thoughtful pager integration with graceful fallback.The
viewWithScrollfunction intelligently handles pager usage - it tries to use the pager when TTY support is available and paging is enabled, but gracefully falls back to standard output if the pager fails. This provides a good user experience while maintaining reliability.
192-226: Comprehensive Pro API integration for uploads.The upload functionality is well-implemented with proper URL parsing, API client initialization, and structured request building. The integration with the Pro API follows good patterns and includes appropriate error handling.
2071474 to
1ac49c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/describe_affected_test.go (1)
111-121: Simplify the complex nested panic handling.The static analysis warning about complexity is valid here. Consider extracting the panic verification into a helper function to improve readability.
+func assertPanic(t *testing.T, expectedMessage string, fn func()) { + defer func() { + if r := recover(); r != nil { + if fmt.Sprintf("%v", r) != expectedMessage { + t.Errorf("Expected panic message %q, got %v", expectedMessage, r) + } + } else { + t.Error("Expected panic but none occurred") + } + }() + fn() +} - if tt.expectedPanic { - defer func() { - if r := recover(); r != nil { - if fmt.Sprintf("%v", r) != tt.panicMessage { - t.Errorf("Expected panic message %q, got %v", tt.panicMessage, r) - } - } else { - t.Error("Expected panic but none occurred") - } - }() - } + if tt.expectedPanic { + assertPanic(t, tt.panicMessage, func() { + gotDescribe := &exec.DescribeAffectedCmdArgs{} + setFlagValueInCliArgs(fs, gotDescribe) + }) + return + }🧰 Tools
🪛 golangci-lint (1.64.8)
[warning] 111-111:
if tt.expectedPanichas complex nested blocks (complexity: 4)(nestif)
internal/exec/describe_affected.go (1)
43-95: Consider grouping related function dependencies.While the dependency injection pattern is excellent for testability, the large number of function fields could become unwieldy. Consider grouping related functions into smaller interfaces.
For example, group execution functions:
type ExecutionMethods interface { ExecuteWithTargetRepoPath(...) (...) ExecuteWithTargetRefClone(...) (...) ExecuteWithTargetRefCheckout(...) (...) } type RenderingMethods interface { PrintOrWriteToFile(...) error AddDependentsToAffected(...) error } type describeAffectedExec struct { atmosConfig *schema.AtmosConfiguration execution ExecutionMethods rendering RenderingMethods pageCreator pager.PageCreator isTTYSupport func() bool }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/describe_affected.go(2 hunks)cmd/describe_affected_test.go(1 hunks)internal/exec/describe_affected.go(5 hunks)internal/exec/describe_affected_test.go(1 hunks)internal/exec/file_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/exec/file_utils.go
- internal/exec/describe_affected_test.go
- cmd/describe_affected.go
🧰 Additional context used
🧠 Learnings (1)
cmd/describe_affected_test.go (4)
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
🪛 golangci-lint (1.64.8)
cmd/describe_affected_test.go
[warning] 111-111: if tt.expectedPanic has complex nested blocks (complexity: 4)
(nestif)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
cmd/describe_affected_test.go (3)
14-24: LGTM! Clean test setup with proper mocking.Good use of dependency injection pattern for testing. The mock setup and expectation verification follow best practices.
26-129: Excellent comprehensive test coverage for flag parsing.The test cases cover all the important scenarios including edge cases like the upload flag's cascading behavior and default value handling. Well-structured table-driven tests.
🧰 Tools
🪛 golangci-lint (1.64.8)
[warning] 111-111:
if tt.expectedPanichas complex nested blocks (complexity: 4)(nestif)
131-164: Good test for unsupported flag type panic behavior.The test correctly verifies that unsupported flag types trigger a panic with the expected message. The manual flag processing logic here mirrors the actual implementation effectively.
internal/exec/describe_affected.go (5)
38-41: Excellent interface design with mock generation.Clean interface definition that supports the dependency injection pattern. The go:generate directive ensures mocks stay up-to-date.
112-171: Well-structured execution flow with proper error handling.The switch-based execution mode selection is clean and handles all the different ways the command can be invoked. Good error propagation throughout.
173-190: Nice separation of query evaluation and rendering logic.The view method cleanly separates custom query evaluation from standard output rendering, making the code easier to follow and test.
228-257: Great paging implementation that addresses the PR objective.The
viewWithScrollfunction effectively implements the pager functionality mentioned in the PR objectives. The fallback to standard output when paging fails is a good design choice.
268-294: Solid pager integration with proper TTY detection.The
viewConfigfunction properly checks for TTY support and handles different output formats appropriately. Good error handling for unsupported formats.
dev-3182-add-pager-to-atmos-describe-affected-command
1ac49c1 to
d3ff60c
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/describe_affected_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/describe_affected_test.go (4)
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
🪛 golangci-lint (1.64.8)
cmd/describe_affected_test.go
[warning] 111-111: if tt.expectedPanic has complex nested blocks (complexity: 4)
(nestif)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
cmd/describe_affected_test.go (3)
14-24: Solid integration test setup.The test properly mocks the execution interface and verifies the command flow. Good use of dependency injection through the factory function.
26-80: Well-structured table-driven tests with good coverage.The test cases cover the main scenarios including flag combinations, upload behavior, and default values. The test structure is clean and maintainable.
122-131: Clean test execution and assertions.Good pattern of setting up the args struct and using testify assertions for clear test output.
…-affected-command
…ommand' of https://github.com/cloudposse/atmos into feature/dev-3182-add-pager-to-atmos-describe-affected-command
d1244ca
…-affected-command
what
why
references
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Style