Add pager to atmos describe affected command #1236
Conversation
samtholiya
added
minor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/dev-3162-add-pager-to-atmos-describe-component-command #1236 +/- ##
===================================================================================================
+ Coverage 32.89% 48.38% +15.48%
===================================================================================================
Files 223 229 +6
Lines 23791 24602 +811
===================================================================================================
+ Hits 7827 11904 +4077
+ Misses 14798 11111 -3687
- Partials 1166 1587 +421
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samtholiya
changed the title
…ommand' of https://github.com/cloudposse/atmos into feature/dev-3182-add-pager-to-atmos-describe-affected-command
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
…pager-to-atmos-describe-affected-command
…ommand' of https://github.com/cloudposse/atmos into feature/dev-3182-add-pager-to-atmos-describe-affected-command
📝 Walkthrough## Walkthrough
This change introduces a terminal pager to several `describe` commands in the CLI, notably `describe affected`, `describe component`, and `describe config`. It adds a pager flag, updates configuration and environment variable support for paging, refactors command execution for modularity, and implements a Bubble Tea-based pager with syntax highlighting. Tests and documentation are updated accordingly.
## Changes
| File(s) / Path(s) | Change Summary |
|---------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `cmd/describe.go`, `cmd/describe_component.go`, `cmd/describe_config.go` | Added/updated `--pager` flag and refactored command execution to support paging and explicit flag parsing. |
| `cmd/describe_affected.go` | Refactored CLI argument parsing, added pager flag, and improved validation. |
| `internal/exec/describe_affected.go`, `internal/exec/describe_component.go`, `internal/exec/describe_config.go` | Refactored execution logic into new structs with pager integration, modularized output handling, added syntax highlighting, and improved TTY detection. |
| `internal/exec/file_utils.go`, `pkg/utils/highlight_utils.go`, `pkg/utils/json_utils.go`, `pkg/utils/yaml_utils.go` | Updated function signatures to accept config pointers and support syntax highlighting for paged output. |
| `pkg/pager/pager.go`, `pkg/pager/model.go`, `pkg/pager/mock_pager.go`, `pkg/pager/model_test.go` | Introduced Bubble Tea-based pager implementation, interface, and tests. |
| `pkg/schema/schema.go`, `pkg/schema/schema_test.go` | Changed pager config type to string, added `IsPagerEnabled()` method, and tested pager enablement logic. |
| `pkg/config/default.go`, `pkg/config/load.go` | Updated default pager config to string, added environment variable binding for pager. |
| `website/docs/cli/commands/describe/describe-component.mdx` | Documented new `--pager` flag and usage example. |
| `go.mod` | Updated dependencies to support new pager and highlighting features. |
| `tests/cli_describe_component_test.go`, `tests/describe_test.go`, `internal/exec/describe_component_test.go`, `internal/exec/describe_config_test.go` | Added/updated tests for pager functionality, flag support, and output validation. |
| `tests/snapshots/…`, `tests/test-cases/demo-stacks.yaml` | Updated test snapshots and cases to reflect pager config changes and output. |
| Other files (`internal/exec/atlantis_generate_repo_config.go`, `internal/exec/describe_dependents.go`, etc.) | Updated print/output functions to accept config pointers for highlighting and paging. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant CLI
participant Config
participant Pager
participant Output
User->>CLI: Run describe affected/component/config
CLI->>Config: Load configuration
CLI->>CLI: Parse flags (including --pager)
CLI->>CLI: Execute describe logic (gather data)
CLI->>Output: Format output (YAML/JSON)
alt Pager enabled and TTY supports
CLI->>Pager: Display output with syntax highlighting
Pager->>User: Interactive paged view
else
CLI->>Output: Print or write output directly
Output->>User: Output to stdout or file
endAssessment against linked issues
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
cmd/describe_affected.go (1)
22-149: 💡 Verification agent❓ Verification inconclusive
Add test coverage for new functions
The static analysis indicates that the new code lacks test coverage. Consider adding unit tests for:
parseDescribeAffectedCliArgssetFlagValueInCliArgscheckFlagErrorThis will improve code reliability and help catch regressions.
Let's check if there are any existing tests for similar functions:
🏁 Script executed:
#!/bin/bash # Look for test files that might contain related tests find . -name "*_test.go" | xargs grep -l "parseDescribeAffectedCliArgs\|setFlagValueInCliArgs\|checkFlagError"Length of output: 112
Add unit tests for CLI argument parsing helpers
The following functions in
cmd/describe_affected.gocurrently have no test coverage. Please add unit tests that cover both success and error scenarios:
parseDescribeAffectedCliArgssetFlagValueInCliArgscheckFlagErrorA new test file (e.g.
cmd/describe_affected_test.go) in the same package should include cases for:
- Valid flag combinations
- Invalid
--formatvalues- Conflicting flags (e.g.
--repo-pathwith--ref)- Error propagation from
checkFlagErrorThis will help catch regressions in CLI flag handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-29: cmd/describe_affected.go#L25-L29
Added lines #L25 - L29 were not covered by tests
[warning] 62-66: cmd/describe_affected.go#L62-L66
Added lines #L62 - L66 were not covered by tests
[warning] 68-71: cmd/describe_affected.go#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 73-76: cmd/describe_affected.go#L73-L76
Added lines #L73 - L76 were not covered by tests
[warning] 79-90: cmd/describe_affected.go#L79-L90
Added lines #L79 - L90 were not covered by tests
[warning] 92-100: cmd/describe_affected.go#L92-L100
Added lines #L92 - L100 were not covered by tests
[warning] 102-102: cmd/describe_affected.go#L102
Added line #L102 was not covered by tests
[warning] 105-131: cmd/describe_affected.go#L105-L131
Added lines #L105 - L131 were not covered by tests
[warning] 133-139: cmd/describe_affected.go#L133-L139
Added lines #L133 - L139 were not covered by tests
[warning] 141-141: cmd/describe_affected.go#L141
Added line #L141 was not covered by tests
[warning] 145-147: cmd/describe_affected.go#L145-L147
Added lines #L145 - L147 were not covered by tests🪛 GitHub Check: golangci-lint
[failure] 62-62:
cyclomatic: function parseDescribeAffectedCliArgs has cyclomatic complexity 14 (> max enabled 10)
[failure] 93-93:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid '--format' flag '%s'. Valid values are 'json' (default) and 'yaml'", result.Format)"
[failure] 99-99:
do not define dynamic errors, use wrapped static errors instead: "errors.New("if the '--repo-path' flag is specified, the '--ref', '--sha', '--ssh-key' and '--ssh-key-password' flags can't be used")"
♻️ Duplicate comments (5)
internal/exec/describe_component_test.go (1)
13-13: Fix comment formatting: Add a period at the end.The comment doesn't end with a period, which violates code style conventions.
-// --- Test --- +// --- Test ---🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 13-13:
Comment should end in a periodcmd/describe_component.go (1)
86-92: Sentence should start with a capital letterThe comment should start with a capital letter to follow code style conventions.
-// we prefer to panic because this is a developer error. +// We prefer to panic because this is a developer error.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-90: cmd/describe_component.go#L90
Added line #L90 was not covered by tests🪛 GitHub Check: golangci-lint
[failure] 86-86:
Sentence should start with a capital letterpkg/utils/json_utils.go (1)
24-40:⚠️ Potential issueImproved error handling needed in GetHighlightedJSON
The function ignores highlighting errors and returns success even when highlighting fails.
highlighted, err := HighlightCodeWithConfig(atmosConfig, prettyJSON.String()) if err != nil { - return prettyJSON.String(), nil + return prettyJSON.String(), err }🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 37-37:
error is not nil (line 35) but it returns nilcmd/describe_affected.go (2)
98-100: 🛠️ Refactor suggestionUse predefined errors instead of dynamic errors
Replace the dynamic error with a predefined error constant as flagged by static analysis.
+var ErrIncompatibleRepoPathFlags = errors.New("if the '--repo-path' flag is specified, the '--ref', '--sha', '--ssh-key' and '--ssh-key-password' flags can't be used") +// In validateRepoPathFlags function if result.RepoPath != "" && (result.Ref != "" || result.SHA != "" || result.SSHKeyPath != "" || result.SSHKeyPassword != "") { - return errors.New("if the '--repo-path' flag is specified, the '--ref', '--sha', '--ssh-key' and '--ssh-key-password' flags can't be used") + return ErrIncompatibleRepoPathFlags }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-100: cmd/describe_affected.go#L92-L100
Added lines #L92 - L100 were not covered by tests🪛 GitHub Check: golangci-lint
[failure] 99-99:
do not define dynamic errors, use wrapped static errors instead: "errors.New("if the '--repo-path' flag is specified, the '--ref', '--sha', '--ssh-key' and '--ssh-key-password' flags can't be used")"
92-94: 🛠️ Refactor suggestionUse predefined errors instead of dynamic errors
Replace the dynamic error with a predefined error constant to follow best practices as flagged by static analysis.
+var ErrInvalidFormat = func(format string) error { + return fmt.Errorf("invalid '--format' flag '%s'. Valid values are 'json' (default) and 'yaml'", format) +} +// In validateFormatFlag function if result.Format != "" && result.Format != "yaml" && result.Format != "json" { - return fmt.Errorf("invalid '--format' flag '%s'. Valid values are 'json' (default) and 'yaml'", result.Format) + return ErrInvalidFormat(result.Format) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-100: cmd/describe_affected.go#L92-L100
Added lines #L92 - L100 were not covered by tests🪛 GitHub Check: golangci-lint
[failure] 93-93:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid '--format' flag '%s'. Valid values are 'json' (default) and 'yaml'", result.Format)"
🧹 Nitpick comments (13)
cmd/describe.go (1)
18-18: Pager flag has unclear string parameter usage.The flag uses a string type with "true" as default, but the description only mentions enable/disable functionality. Consider clarifying that the parameter can accept program names like "less" or "more" in addition to boolean values.
- describeCmd.PersistentFlags().String("pager", "true", "Disable / Enable the paging user experience") + describeCmd.PersistentFlags().String("pager", "true", "Control paging: 'true'/'false' to enable/disable, or specify pager name (e.g., 'less', 'more')")internal/exec/atmos.go (1)
111-111: Consider adding tests for these modified linesThe static analysis flagged that these modified lines aren't covered by tests.
Consider adding test cases that verify the correct behavior when calling these functions with the atmosConfig parameter.
Also applies to: 123-123
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 111-111: internal/exec/atmos.go#L111
Added line #L111 was not covered by teststests/describe_test.go (2)
18-32: Good JSON format test with pager configurationThis test properly sets up the test environment, configures the pager via environment variable, and verifies that the command executes successfully with JSON output format.
Consider adding assertions to verify the actual output content matches expectations, not just that the command doesn't error.
err := cmd.Execute() if err != nil { t.Fatalf("Failed to execute command: %v", err) } +// Consider capturing output and verifying content +// output := captureOutput(func() { +// cmd.Execute() +// }) +// assert.Contains(t, output, "expected-json-content")
34-48: Good YAML format test with pager configurationThis test properly sets up the test environment, configures the pager via environment variable, and verifies that the command executes successfully with YAML output format.
As with the JSON test, consider adding assertions to verify the actual output content, not just that the command doesn't error.
err := cmd.Execute() if err != nil { t.Fatalf("Failed to execute command: %v", err) } +// Consider capturing output and verifying content +// output := captureOutput(func() { +// cmd.Execute() +// }) +// assert.Contains(t, output, "expected-yaml-content")cmd/describe_config.go (1)
38-39: Remove TODO comment once PR #1174 is merged.The TODO comment references another PR that might be merged by now. Check if this can be cleaned up.
- // TODO: update this post pr:https://github.com/cloudposse/atmos/pull/1174 is merged atmosConfig.Settings.Terminal.Pager, err = cmd.Flags().GetString("pager")cmd/describe_component.go (2)
25-27: Remove redundant argument validationThis manual argument check is redundant since you're already using
cobra.ExactArgs(1)on line 20, which enforces the same constraint.-if len(args) != 1 { - return errInvalidFlag -}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: cmd/describe_component.go#L26-L27
Added lines #L26 - L27 were not covered by tests
87-92: Consider a less drastic error handling approachWhile panic makes sense for developer errors, consider returning errors instead of crashing the program, especially in a CLI tool. This would make the code more robust and user-friendly.
-// checkFlagNotPresentError checks if the error is nil. -func checkFlagNotPresentError(err error) { - if err != nil { - panic(err) - } +// checkFlagNotPresentError checks if the error is nil and returns it. +func checkFlagNotPresentError(err error) error { + if err != nil { + return fmt.Errorf("flag error: %w", err) + } + return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-90: cmd/describe_component.go#L90
Added line #L90 was not covered by testscmd/describe_affected.go (1)
145-149: Consider less abrupt error handlingUsing
panicfor flag errors could be harsh in a CLI context. Consider returning errors instead for more graceful failure handling.-func checkFlagError(err error) { - if err != nil { - panic(err) - } -} +func checkFlagError(err error) error { + return err +} +// Then in setFlagValueInCliArgs: +var err error +for k := range flagsKeyValue { + if !flags.Changed(k) { + continue + } + switch v := flagsKeyValue[k].(type) { + case *string: + *v, err = flags.GetString(k) + case *bool: + *v, err = flags.GetBool(k) + default: + return fmt.Errorf("unsupported type %T for flag %s", v, k) + } + if err != nil { + return fmt.Errorf("error getting flag %s: %w", k, err) + } +} +return nil🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-147: cmd/describe_affected.go#L145-L147
Added lines #L145 - L147 were not covered by testsinternal/exec/describe_component.go (1)
46-111: Reduce function lengthThe function exceeds the 60-line limit (it's 64 lines). Consider extracting some logic into helper methods for better readability and to satisfy the linter.
You could extract the parameter initialization or the query evaluation logic into separate methods:
func (d *DescribeComponentExec) ExecuteDescribeComponentCmd(describeComponentParams DescribeComponentParams) error { - component := describeComponentParams.Component - stack := describeComponentParams.Stack - processTemplates := describeComponentParams.ProcessTemplates - processYamlFunctions := describeComponentParams.ProcessYamlFunctions - skip := describeComponentParams.Skip - query := describeComponentParams.Query - pager := describeComponentParams.Pager - format := describeComponentParams.Format - file := describeComponentParams.File + params := d.extractParams(describeComponentParams) var err error var atmosConfig schema.AtmosConfiguration atmosConfig, err = d.initCliConfig(schema.ConfigAndStacksInfo{ - ComponentFromArg: component, - Stack: stack, + ComponentFromArg: params.component, + Stack: params.stack, }, true) if err != nil { return err } componentSection, err := d.executeDescribeComponent( - component, - stack, - processTemplates, - processYamlFunctions, - skip, + params.component, + params.stack, + params.processTemplates, + params.processYamlFunctions, + params.skip, ) if err != nil { return err } var res any - if pager != "" { - atmosConfig.Settings.Terminal.Pager = pager + if params.pager != "" { + atmosConfig.Settings.Terminal.Pager = params.pager } - if query != "" { - res, err = d.evaluateYqExpression(&atmosConfig, componentSection, query) - if err != nil { - return err - } - } else { - res = componentSection - } + res, err = d.evaluateQueryIfPresent(&atmosConfig, componentSection, params.query) + if err != nil { + return err + } if atmosConfig.Settings.Terminal.IsPagerEnabled() { - err = d.viewConfig(&atmosConfig, component, format, res) + err = d.viewConfig(&atmosConfig, params.component, params.format, res) switch err.(type) { case DescribeConfigFormatError: return err case nil: return nil default: log.Debug("Failed to use pager") } } - err = d.printOrWriteToFile(&atmosConfig, format, file, res) + err = d.printOrWriteToFile(&atmosConfig, params.format, params.file, res) if err != nil { return err } return nil } +type extractedParams struct { + component string + stack string + processTemplates bool + processYamlFunctions bool + skip []string + query string + pager string + format string + file string +} + +func (d *DescribeComponentExec) extractParams(params DescribeComponentParams) extractedParams { + return extractedParams{ + component: params.Component, + stack: params.Stack, + processTemplates: params.ProcessTemplates, + processYamlFunctions: params.ProcessYamlFunctions, + skip: params.Skip, + query: params.Query, + pager: params.Pager, + format: params.Format, + file: params.File, + } +} + +func (d *DescribeComponentExec) evaluateQueryIfPresent(atmosConfig *schema.AtmosConfiguration, data any, query string) (any, error) { + if query == "" { + return data, nil + } + return d.evaluateYqExpression(atmosConfig, data, query) +}🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 46-46:
function-length: maximum number of lines per function exceeded; max 60 but got 64pkg/utils/highlight_utils.go (1)
75-147: Consider breaking down the HighlightCodeWithConfig functionThis function exceeds the 60-line limit (as flagged by static analysis). Consider extracting some of the lexer determination logic into a separate function.
func HighlightCodeWithConfig(config *schema.AtmosConfiguration, code string, format ...string) (string, error) { if !term.IsTerminal(int(os.Stdout.Fd())) { return code, nil } settings := GetHighlightSettings(config) if !settings.Enabled { return code, nil } // Get terminal width config.Settings.Terminal.MaxWidth = templates.GetTerminalWidth() - // Determine lexer based on format flag or content format - var lexerName string - if len(format) > 0 && format[0] != "" { - // Use format flag if provided - lexerName = strings.ToLower(format[0]) - } else { - // This is just a fallback - trimmed := strings.TrimSpace(code) - - // Try to parse as JSON first - if json.Valid([]byte(trimmed)) { - lexerName = "json" - } else { - // Check for common YAML indicators - // 1. Contains key-value pairs with colons - // 2. Does not start with a curly brace (which could indicate malformed JSON) - // 3. Contains indentation or list markers - if (strings.Contains(trimmed, ":") && !strings.HasPrefix(trimmed, "{")) || - strings.Contains(trimmed, "\n ") || - strings.Contains(trimmed, "\n- ") { - lexerName = "yaml" - } else { - // Fallback to plaintext if format is unclear - lexerName = "plaintext" - } - } - } + lexerName := determineLexer(code, format...) // Get lexer lexer := lexers.Get(lexerName) if lexer == nil { lexer = lexers.Fallback } // Get style s := styles.Get(settings.Theme) if s == nil { s = styles.Fallback } // Get formatter var formatter chroma.Formatter if settings.LineNumbers { formatter = formatters.TTY256 } else { formatter = formatters.Get(settings.Formatter) if formatter == nil { formatter = formatters.Fallback } } // Create buffer for output var buf bytes.Buffer // Format the code iterator, err := lexer.Tokenise(nil, code) if err != nil { return code, err } err = formatter.Format(&buf, s, iterator) if err != nil { return code, err } return buf.String(), nil } +// determineLexer determines the appropriate lexer name based on format or content +func determineLexer(code string, format ...string) string { + // Use format flag if provided + if len(format) > 0 && format[0] != "" { + return strings.ToLower(format[0]) + } + + // Otherwise, try to determine from content + trimmed := strings.TrimSpace(code) + + // Try to parse as JSON first + if json.Valid([]byte(trimmed)) { + return "json" + } + + // Check for common YAML indicators + // 1. Contains key-value pairs with colons + // 2. Does not start with a curly brace (which could indicate malformed JSON) + // 3. Contains indentation or list markers + if (strings.Contains(trimmed, ":") && !strings.HasPrefix(trimmed, "{")) || + strings.Contains(trimmed, "\n ") || + strings.Contains(trimmed, "\n- ") { + return "yaml" + } + + // Fallback to plaintext if format is unclear + return "plaintext" +}🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 75-75:
function-length: maximum number of lines per function exceeded; max 60 but got 71internal/exec/describe_config.go (1)
55-66: Pager skips--outputwritingWhen the pager is enabled and there’s no error, the function returns early (line 61).
This meansoutputis silently ignored when the user asks for both paged viewing and file export.A small tweak lets us keep today’s UX while still honoring
--output:- case nil: - return nil + case nil: + if output == "" { + return nil // pure pager workflow + } + // fall-through to file writerThis keeps behaviour unchanged for the majority case yet still writes the file when requested.
pkg/pager/model.go (1)
80-82: Forward reference is safe but can be sharper
statusBarMessageHelpStylerelies on thegreenconstant declared later.
Go handles this at compile-time, yet grouping the colour declarations first would improve readability and avoid head-scratchers for new contributors.internal/exec/describe_affected.go (1)
219-328: Duplicate legacy function
ExecuteDescribeAffectedCmdduplicates the new struct’s logic almost verbatim.
Unless kept for backwards compatibility, this increases maintenance cost and test surface.
Evaluate deprecating it or delegating toDescribeAffectedExec.Execute.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
Makefile(1 hunks)cmd/describe.go(1 hunks)cmd/describe_affected.go(3 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(2 hunks)cmd/docs.go(1 hunks)go.mod(2 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/atmos.go(2 hunks)internal/exec/describe_affected.go(5 hunks)internal/exec/describe_component.go(2 hunks)internal/exec/describe_component_test.go(1 hunks)internal/exec/describe_config.go(1 hunks)internal/exec/describe_config_test.go(1 hunks)internal/exec/describe_dependents.go(1 hunks)internal/exec/describe_stacks.go(1 hunks)internal/exec/describe_workflows.go(1 hunks)internal/exec/file_utils.go(2 hunks)pkg/atlantis/atlantis_generate_repo_config_test.go(2 hunks)pkg/config/default.go(1 hunks)pkg/config/load.go(2 hunks)pkg/pager/mock_pager.go(1 hunks)pkg/pager/model.go(1 hunks)pkg/pager/model_test.go(1 hunks)pkg/pager/pager.go(1 hunks)pkg/schema/schema.go(1 hunks)pkg/schema/schema_test.go(1 hunks)pkg/utils/highlight_utils.go(3 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/json_utils_test.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/utils/yq_test.go(5 hunks)pkg/utils/yq_utils.go(1 hunks)tests/cli_describe_component_test.go(1 hunks)tests/describe_test.go(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)tests/test-cases/demo-stacks.yaml(3 hunks)website/docs/cli/commands/describe/describe-component.mdx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
internal/exec/describe_stacks.go (1)
Learnt from: Cerebrovinny
PR: cloudposse/atmos#764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
internal/exec/describe_affected.go (1)
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
🧬 Code Graph Analysis (10)
pkg/atlantis/atlantis_generate_repo_config_test.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAML(44-51)
internal/exec/atmos.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAML(44-51)
pkg/utils/yq_test.go (2)
pkg/utils/yaml_utils.go (1)
PrintAsYAML(44-51)pkg/utils/yq_utils.go (1)
EvaluateYqExpression(46-80)
pkg/utils/yq_utils.go (2)
pkg/schema/schema.go (1)
AtmosConfiguration(13-46)pkg/utils/yaml_utils.go (2)
ConvertToYAML(89-95)UnmarshalYAML(222-224)
internal/exec/file_utils.go (3)
pkg/schema/schema.go (1)
AtmosConfiguration(13-46)pkg/utils/yaml_utils.go (1)
PrintAsYAML(44-51)pkg/utils/json_utils.go (1)
PrintAsJSON(15-22)
internal/exec/atlantis_generate_repo_config.go (1)
pkg/utils/yaml_utils.go (1)
PrintAsYAML(44-51)
pkg/schema/schema_test.go (1)
pkg/schema/schema.go (1)
Terminal(190-196)
internal/exec/describe_config_test.go (4)
internal/exec/describe_config.go (2)
DescribeConfigFormatError(16-18)NewDescribeConfig(33-40)pkg/schema/schema.go (6)
AtmosConfiguration(13-46)Components(296-299)Terraform(266-277)Settings(746-750)AtmosSettings(212-228)Terminal(190-196)pkg/pager/mock_pager.go (1)
NewMockPageCreator(25-29)internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStdout(83-87)
cmd/describe_component.go (3)
internal/exec/describe_component.go (2)
NewDescribeComponentExec(35-44)DescribeComponentParams(14-24)pkg/utils/markdown_utils.go (1)
PrintErrorMarkdownAndExit(87-89)cmd/cmd_utils.go (2)
ComponentsArgCompletion(755-777)AddStackCompletion(748-753)
pkg/utils/highlight_utils.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(13-46)SyntaxHighlighting(202-210)
🪛 GitHub Check: codecov/patch
cmd/docs.go
[warning] 105-105: cmd/docs.go#L105
Added line #L105 was not covered by tests
internal/exec/atmos.go
[warning] 111-111: internal/exec/atmos.go#L111
Added line #L111 was not covered by tests
[warning] 123-123: internal/exec/atmos.go#L123
Added line #L123 was not covered by tests
internal/exec/atlantis_generate_repo_config.go
[warning] 528-528: internal/exec/atlantis_generate_repo_config.go#L528
Added line #L528 was not covered by tests
cmd/describe_component.go
[warning] 26-27: cmd/describe_component.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 90-90: cmd/describe_component.go#L90
Added line #L90 was not covered by tests
cmd/describe_config.go
[warning] 19-25: cmd/describe_config.go#L19-L25
Added lines #L19 - L25 were not covered by tests
[warning] 27-30: cmd/describe_config.go#L27-L30
Added lines #L27 - L30 were not covered by tests
[warning] 32-35: cmd/describe_config.go#L32-L35
Added lines #L32 - L35 were not covered by tests
[warning] 37-42: cmd/describe_config.go#L37-L42
Added lines #L37 - L42 were not covered by tests
[warning] 45-45: cmd/describe_config.go#L45
Added line #L45 was not covered by tests
[warning] 49-49: cmd/describe_config.go#L49
Added line #L49 was not covered by tests
cmd/describe_affected.go
[warning] 25-29: cmd/describe_affected.go#L25-L29
Added lines #L25 - L29 were not covered by tests
[warning] 62-66: cmd/describe_affected.go#L62-L66
Added lines #L62 - L66 were not covered by tests
[warning] 68-71: cmd/describe_affected.go#L68-L71
Added lines #L68 - L71 were not covered by tests
[warning] 73-76: cmd/describe_affected.go#L73-L76
Added lines #L73 - L76 were not covered by tests
[warning] 79-90: cmd/describe_affected.go#L79-L90
Added lines #L79 - L90 were not covered by tests
[warning] 92-100: cmd/describe_affected.go#L92-L100
Added lines #L92 - L100 were not covered by tests
[warning] 102-102: cmd/describe_affected.go#L102
Added line #L102 was not covered by tests
[warning] 105-131: cmd/describe_affected.go#L105-L131
Added lines #L105 - L131 were not covered by tests
[warning] 133-139: cmd/describe_affected.go#L133-L139
Added lines #L133 - L139 were not covered by tests
[warning] 141-141: cmd/describe_affected.go#L141
Added line #L141 was not covered by tests
[warning] 145-147: cmd/describe_affected.go#L145-L147
Added lines #L145 - L147 were not covered by tests
internal/exec/describe_affected.go
[warning] 94-103: internal/exec/describe_affected.go#L94-L103
Added lines #L94 - L103 were not covered by tests
[warning] 107-151: internal/exec/describe_affected.go#L107-L151
Added lines #L107 - L151 were not covered by tests
[warning] 155-155: internal/exec/describe_affected.go#L155
Added line #L155 was not covered by tests
[warning] 159-160: internal/exec/describe_affected.go#L159-L160
Added lines #L159 - L160 were not covered by tests
[warning] 162-162: internal/exec/describe_affected.go#L162
Added line #L162 was not covered by tests
[warning] 166-171: internal/exec/describe_affected.go#L166-L171
Added lines #L166 - L171 were not covered by tests
[warning] 173-186: internal/exec/describe_affected.go#L173-L186
Added lines #L173 - L186 were not covered by tests
🪛 GitHub Check: golangci-lint
internal/exec/describe_component_test.go
[failure] 13-13:
Comment should end in a period
cmd/describe_component.go
[failure] 86-86:
Sentence should start with a capital letter
cmd/describe_affected.go
[failure] 62-62:
cyclomatic: function parseDescribeAffectedCliArgs has cyclomatic complexity 14 (> max enabled 10)
[failure] 93-93:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid '--format' flag '%s'. Valid values are 'json' (default) and 'yaml'", result.Format)"
[failure] 99-99:
do not define dynamic errors, use wrapped static errors instead: "errors.New("if the '--repo-path' flag is specified, the '--ref', '--sha', '--ssh-key' and '--ssh-key-password' flags can't be used")"
internal/exec/describe_component.go
[failure] 46-46:
function-length: maximum number of lines per function exceeded; max 60 but got 64
pkg/utils/json_utils.go
[failure] 37-37:
error is not nil (line 35) but it returns nil
pkg/utils/highlight_utils.go
[failure] 75-75:
function-length: maximum number of lines per function exceeded; max 60 but got 71
internal/exec/describe_affected.go
[failure] 107-107:
cognitive complexity 28 of func (*DescribeAffectedExec).Execute is high (> 20)
[failure] 112-112:
ifElseChain: rewrite if-else to switch statement
[warning] 166-166:
if a.Query == "" has complex nested blocks (complexity: 13)
[failure] 218-218:
cognitive complexity 28 of func ExecuteDescribeAffectedCmd is high (> 20)
[failure] 341-341:
hugeParam: v is heavy (104 bytes); consider passing it by pointer
[failure] 370-370:
hugeParam: v is heavy (80 bytes); consider passing it by pointer
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (73)
Makefile (1)
64-64: Good improvement to test coverage.Adding the
-coverpkg=./...flag ensures coverage measurement across all packages in the module, not just those being directly tested. This will give you better visibility into how the new pager functionality is being exercised throughout the codebase.tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
113-113: Properly updated pager configuration.The change from boolean
trueto string"1"for the pager setting aligns with the broader refactoring to support multiple pager options. This is the right update to reflect the new string-based configuration approach.tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
70-70: Correctly updated pager configuration format.The change from boolean
falseto string"0"properly reflects the new string-based approach for pager configuration. This is consistent with the implementation changes in the codebase.tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden (1)
16-16: Added appropriate debug message for pager failures.This new debug log line provides visibility into pager usage failures, which is important for troubleshooting. This aligns well with the implementation of the new pager functionality.
pkg/config/default.go (1)
58-58: Good update from boolean to string for pager configuration.Changing
Pagerfrom a boolean to string value "less" makes the configuration more flexible, allowing users to specify their preferred pager. This is a common pattern in CLI tools that aligns with standard Unix practices.tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden (1)
20-20: Appropriate debug message for pager failure.The added debug message provides helpful diagnostic information when the pager fails to initialize or run, which improves troubleshooting.
tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden (2)
48-60: Well-structured terminal configuration section.The terminal configuration section is comprehensive and includes all necessary options for controlling the terminal display, paging, and syntax highlighting capabilities.
50-50:✅ Verification successful
Verify pager value in test snapshot.
The pager is set to "1" in this test snapshot. Ensure this value is intentional and documented, as it differs from both the default "less" in the code and typical boolean strings like "true"/"false".
🏁 Script executed:
#!/bin/bash # Check if "1" is a valid pager setting according to the codebase # Look for code that handles pager values rg -A 5 -B 5 "IsPagerEnabled|pager.*value"Length of output: 5222
Pager value validated
The snapshot’spager: "1"maps to an enabled pager—IsPagerEnabled()explicitly treats"1"as valid. No changes required.go.mod (1)
13-13: Direct dependency declaration for terminal pager components.The change correctly reclassifies these dependencies from indirect to direct. This aligns with the new pager functionality implementation that directly uses these libraries for terminal UI rendering, ANSI styling, and clipboard support.
Also applies to: 51-51, 55-56
tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden (1)
35-47: Added terminal configuration section with pager settings.The new terminal configuration section properly defines default values for the pager and related settings. This snapshot update reflects the newly added terminal pager functionality.
tests/test-cases/demo-stacks.yaml (3)
77-78: Disabled pager in docs command test.Setting
ATMOS_PAGER: falseensures consistent test output by preventing the pager from activating during test execution.
96-97: Disabled pager in describe config YAML test.Setting
ATMOS_PAGER: falseensures consistent test output by preventing the pager from activating during test execution.
118-119: Disabled pager in describe config JSON test.Setting
ATMOS_PAGER: falseensures consistent test output by preventing the pager from activating during test execution.pkg/atlantis/atlantis_generate_repo_config_test.go (1)
18-18: Updated PrintAsYAML calls with configuration parameter.The function calls have been properly updated to include the atmosConfig parameter, matching the refactored PrintAsYAML function signature. This change is necessary to support the new terminal pager functionality.
Also applies to: 40-40
website/docs/cli/commands/describe/describe-component.mdx (2)
58-59: Good addition of pager example.This example clearly shows users how to invoke the component description with pager functionality using the
morepager tool.
79-79: Documentation for pager flag is complete.The added
--pagerflag documentation clearly explains its purpose to enable/disable paging.internal/exec/describe_stacks.go (1)
139-139: Updated function call to support configuration-aware output.The change properly integrates the
describe stackscommand with the new pager functionality by passing the configuration pointer to theprintOrWriteToFilefunction.cmd/docs.go (1)
105-109: Good usage of new pager configuration logic.The code now uses the new
IsPagerEnabled()method while maintaining backward compatibility with the deprecatedsettings.docs.paginationsetting.Since this line was flagged as not covered by tests, consider adding a test case that verifies the pager enablement logic for the docs command:
#!/bin/bash # Verify test coverage for the IsPagerEnabled method in docs.go # Look for existing tests that might be testing related functionality rg -t go "Test.*Docs.*Pager" --glob "**/*_test.go" # Check if the IsPagerEnabled method has test coverage elsewhere rg -t go "Test.*IsPagerEnabled" --glob "**/*_test.go"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-105: cmd/docs.go#L105
Added line #L105 was not covered by testspkg/schema/schema.go (2)
192-192: Good improvement from bool to string type for pager setting.Changing the
Pagerfield type from boolean to string adds flexibility, allowing users to specify which pager program to use instead of just turning it on or off.
198-200: Well-implemented pager enablement logic.The
IsPagerEnabled()method provides a clean interface to determine if paging should be enabled based on string values, supporting multiple affirmative options ("on", "less", "true", etc.). This maintains backward compatibility while adding new capabilities.internal/exec/atmos.go (2)
123-123: Added atmosConfig parameter correctly to PrintAsYAMLThe change passes the configuration to the YAML printer, enabling syntax highlighting and paging functionality for the describe dependents command output.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 123-123: internal/exec/atmos.go#L123
Added line #L123 was not covered by tests
111-111:✅ Verification successful
Added atmosConfig parameter correctly to PrintAsYAML
The change passes the configuration to the YAML printer, aligning with the new pager feature. This enables syntax highlighting and paging functionality for the describe component command output.
🏁 Script executed:
#!/bin/bash # Verify if all PrintAsYAML calls have been updated with the new parameter rg "PrintAsYAML\([^&]" --type=goLength of output: 962
All PrintAsYAML calls include the new atmosConfig parameter
Verified that every invocation ofPrintAsYAMLnow accepts theatmosConfigargument:
- internal/exec/atmos.go (uses
u.PrintAsYAML(&atmosConfig, data))- internal/exec/file_utils.go (passes the pointer variable)
- All tests in pkg/utils/yq_test.go
No further updates required.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 111-111: internal/exec/atmos.go#L111
Added line #L111 was not covered by testsinternal/exec/describe_workflows.go (1)
79-79: Added atmosConfig parameter correctly to printOrWriteToFileThe change passes the configuration to the file utility function, allowing the describe workflows command to use the new pager functionality.
internal/exec/file_utils.go (3)
35-35: Added atmosConfig parameter correctly to printOrWriteToFile function signatureThe function signature has been updated to accept the atmosConfig parameter, which is necessary for the pager functionality.
43-43: Properly passes atmosConfig to PrintAsYAMLThe change correctly forwards the atmosConfig to the YAML printer when outputting to console.
56-56: Properly passes atmosConfig to PrintAsJSONThe change correctly forwards the atmosConfig to the JSON printer when outputting to console.
pkg/utils/json_utils_test.go (1)
9-33: Good test coverage for PrintAsJSON with the new parameterThe test verifies that PrintAsJSON works correctly with the new atmosConfig parameter, including terminal syntax highlighting settings.
pkg/config/load.go (2)
103-103: Configuration environment variable binding for pager looks goodBinding the terminal pager setting to both
ATMOS_PAGERand systemPAGERenvironment variables provides flexible configuration options for users.
121-121: Default configuration for pager is appropriateSetting the default pager value to
trueensures users get a good out-of-box experience with the new pager feature without requiring explicit configuration.pkg/schema/schema_test.go (1)
40-71: Comprehensive test coverage for IsPagerEnabled methodThe test covers all relevant string values that users might provide for the pager setting. It properly tests both enabling and disabling scenarios, including case sensitivity and empty string handling. This provides good confidence in the string interpretation logic.
pkg/utils/yq_test.go (1)
68-68: Function signature update consistently appliedAll calls to
PrintAsYAMLhave been correctly updated to include theatmosConfigparameter, aligning with the function signature change inpkg/utils/yaml_utils.go.Also applies to: 76-76, 84-84, 92-92, 100-100, 108-108, 120-120, 132-132, 144-144, 155-155, 161-161, 167-167, 173-173
tests/cli_describe_component_test.go (1)
1-19: Good test case for the new pager featureThis test verifies that the describe component command works correctly with the new
--pagerflag set to "more" and YAML output format. The test properly sets up the CLI arguments and verifies successful execution.internal/exec/atlantis_generate_repo_config.go (1)
528-528:✅ Verification successful
Function signature change to PrintAsYAML now requires atmosConfig parameter
The signature change to
u.PrintAsYAMLadds the ability to use CLI configuration for output formatting and paging support. This is consistent with broader changes throughout the codebase.
🏁 Script executed:
#!/bin/bash # Verify test coverage for this line grep -n "PrintAsYAML.*&atmosConfig" internal/exec/atlantis_generate_repo_config_test.go || echo "Test coverage needed for PrintAsYAML with atmosConfig parameter at line 528"Length of output: 335
🏁 Script executed:
#!/bin/bash # Search for PrintAsYAML usage in any test files across the repo rg -n 'PrintAsYAML' -g '*_test.go' || echo "No PrintAsYAML coverage found in any test"Length of output: 1045
Verified test coverage for PrintAsYAML signature change
Confirmed that the updated call is covered in the existing tests:
- pkg/atlantis/atlantis_generate_repo_config_test.go:40
err = u.PrintAsYAML(&atmosConfig, atlantisYaml)All set—no additional test needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 528-528: internal/exec/atlantis_generate_repo_config.go#L528
Added line #L528 was not covered by testsinternal/exec/describe_dependents.go (1)
79-79: Update to printOrWriteToFile now requires configuration parameterThe change adds a pointer to
atmosConfigas the first parameter toprintOrWriteToFile. This aligns with the refactoring work to provide CLI configuration context for output formatting and paging support.pkg/utils/yq_utils.go (1)
82-116: Good addition of type-safe YQ expression evaluatorThe new
EvaluateYqExpressionWithTypefunction adds type safety by using generics, providing a cleaner API for callers that know the expected return type. This is a solid improvement over the untyped version.A few observations:
- The function properly returns a pointer to the evaluated result
- Error messages are clearly labeled with the function name
- Implementation closely follows the existing pattern for consistency
tests/describe_test.go (1)
10-16: Useful helper function for CLI command testingThe
ExecuteCommandfunction provides a clean way to test CLI commands by setting command line arguments and executing the main CLI command.internal/exec/describe_component_test.go (1)
14-125: Test implementation looks good!The test function provides thorough coverage of the pager functionality with various formats and configurations. Good use of mocks and assertions to verify behavior.
pkg/utils/yaml_utils.go (2)
44-51: Good refactoring of YAML output handling.The function now cleanly delegates YAML conversion and highlighting to the new helper function while maintaining the same interface. This separation of concerns improves maintainability.
53-63: Well-structured new helper function.The
GetHighlightedYAMLfunction properly handles the conversion and highlighting process with appropriate error handling, returning plain YAML as a fallback if highlighting fails.pkg/pager/pager.go (3)
8-11: Good use of interface for testability.Defining a
PageCreatorinterface and generating mocks with gomock enables thorough testing of code that depends on this functionality.
13-21: Solid implementation with dependency injection.The
pageCreatorstruct and constructor allow for dependency injection of the tea.Program creator, which improves testability.
23-37: Well-implemented pager with good terminal UI options.The
Runmethod creates a Bubble Tea program with appropriate options for alternate screen buffer and mouse tracking, enhancing the user experience.cmd/describe_config.go (1)
19-49:❌ Incorrect review comment
Improved error handling and integration of pager functionality.
The command now properly handles errors and integrates with the pager functionality. Good use of explicit flag parsing and config initialization.
The implementation is missing test coverage. Consider adding tests for this command to ensure reliability:
🏁 Script executed:
#!/bin/bash # Check for test files that might test this code fd -t f "test.*describe_config" -x cat {} \; | grep -i "test.*describeconfig"Length of output: 82
Let’s dig deeper to confirm test coverage for the describe_config command:
🏁 Script executed:
#!/bin/bash # Search for any Go test files referencing describe_config or NewDescribeConfig rg -n "describe_config" -g "*_test.go" || true rg -n "DescribeConfig" -g "*_test.go" || true rg -n "NewDescribeConfig" -g "*_test.go" || trueLength of output: 2017
Tests for describe_config already exist
The CLI behavior in cmd/describe_config.go is exercised by existing unit tests in internal/exec/describe_config_test.go (e.g. TestDescribeConfig, NewDescribeConfig, ExecuteDescribeConfigCmd in various scenarios). No additional test files are required for core logic coverage.• internal/exec/describe_config_test.go
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-25: cmd/describe_config.go#L19-L25
Added lines #L19 - L25 were not covered by tests
[warning] 27-30: cmd/describe_config.go#L27-L30
Added lines #L27 - L30 were not covered by tests
[warning] 32-35: cmd/describe_config.go#L32-L35
Added lines #L32 - L35 were not covered by tests
[warning] 37-42: cmd/describe_config.go#L37-L42
Added lines #L37 - L42 were not covered by tests
[warning] 45-45: cmd/describe_config.go#L45
Added line #L45 was not covered by tests
[warning] 49-49: cmd/describe_config.go#L49
Added line #L49 was not covered by testspkg/pager/mock_pager.go (1)
1-49: Mock implementation meets requirements for testingThis mock implementation of the
PageCreatorinterface provides the necessary functionality for testing components that depend on paging behavior.pkg/pager/model_test.go (5)
14-18: Good test coverage for model initializationThe Init test verifies the model initialization works as expected.
20-81: Comprehensive test suite for model updatesTest cases effectively cover key press handling, window sizing (both when ready and not ready), and viewport updates. This ensures the pager's core interaction logic works properly.
83-106: View rendering tests cover all statesTests appropriately check both the not-ready initialization state and ready state with content rendering.
108-132: Footer view tests validate scroll percentage displayTests verify the footer displays correct scroll information in different viewport scenarios.
134-163: Helper function tests ensure utility functions work properlyThe max function and styling tests confirm the supporting UI elements function correctly.
cmd/describe_component.go (4)
12-12: Create a defined static errorGood approach using a defined static error rather than creating dynamic errors in the function.
20-21: Improved command argument validationUsing
cobra.ExactArgs(1)enhances the command's validation by enforcing exactly one argument.
29-60: Structured parameter handling improves maintainabilityThe refactored approach of explicitly retrieving all flags and passing them as a structured params object makes the code more maintainable and testable.
72-72: Simplified flag descriptionThe format flag description is now cleaner.
pkg/utils/json_utils.go (2)
15-22: Configuration-aware JSON printingGood update to accept configuration for customizing the output format.
42-60: Consistent fallback approach for AtmosConfig JSONThis function correctly implements a fallback mechanism by checking for nil error explicitly.
internal/exec/describe_config_test.go (8)
1-15: Well-structured test setup for DescribeConfigFormatErrorThe test case effectively validates the error message format for the
DescribeConfigFormatErrortype. This is a good practice to ensure consistent error messaging.
17-37: Good test coverage for constructor and initializationThe test cases validate both the correct creation of the
describeConfigExecobject and the proper initialization of its fields.Tests ensure that:
- The config is properly assigned
- The pageCreator is initialized
- The printOrWriteToFile function is set
This provides good verification of the initialization logic.
39-54: Solid mocking approach for pager testing with TTY supportThis test case effectively:
- Mocks the pager dependency
- Sets expectations for the pager's behavior
- Simulates TTY availability
- Tests the YAML format output path
The use of gomock provides clear validation of the pager interaction.
56-70: JSON format test ensures format flexibilityThis test complements the YAML test by ensuring the JSON output format is properly handled with TTY support. The consistent pattern between test cases enhances readability.
72-84: Error handling validation for invalid formatsThe test properly validates that the function correctly returns a
DescribeConfigFormatErrorwhen an invalid format is provided, ensuring robust input validation.
86-100: Thorough testing of non-TTY output pathThis test verifies the fallback behavior when TTY is not supported, using a mock function for
printOrWriteToFilethat validates the expected arguments and returns the expected value. The assertions confirm that the right format, file, and data are passed to the output function.
102-116: Test case for query executionThis test verifies the execution path when a query parameter is provided. The test structure follows the established pattern seen in other test cases.
118-127: Edge case validation for YQ expression evaluation errorsThis test ensures proper error handling when a malformed query is provided, verifying that the appropriate error message is returned, which helps with debugging.
cmd/describe_affected.go (1)
22-30: Improved separation of concernsThe refactoring separates CLI argument parsing from execution logic, which improves code modularity. This aligns with the broader changes in the PR to introduce a pager feature.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-29: cmd/describe_affected.go#L25-L29
Added lines #L25 - L29 were not covered by testsinternal/exec/describe_component.go (4)
14-24: Well-structured parameter structCreating a dedicated
DescribeComponentParamsstruct is a clear improvement over passing numerous individual parameters. This makes the code more maintainable and the function signature cleaner.
26-33: Good dependency management with struct fieldsThe
DescribeComponentExecstruct encapsulates dependencies effectively, making testing easier through dependency injection and improving the overall architecture.
35-44: Clean constructor with sensible defaultsThe constructor provides appropriate default implementations while still allowing for dependency injection during testing.
113-139: Well-implemented viewConfig methodThis method effectively:
- Checks for TTY support
- Formats the output based on the specified format
- Uses the pager to display the output
The error handling is comprehensive and the method follows a clear logical flow.
pkg/utils/highlight_utils.go (3)
33-33: Appropriate parameter type changeChanging from value to pointer parameter for
AtmosConfigurationis a good performance improvement, avoiding unnecessary struct copying for this potentially large struct.
75-75: Consistent parameter order in HighlightCodeWithConfigMoving the configuration parameter to the first position is a good practice for consistency across the codebase.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 75-75:
function-length: maximum number of lines per function exceeded; max 60 but got 71
175-175: Proper pointer usage in struct methodUsing a pointer to the struct's config field is appropriate here to avoid unnecessary copying and align with the updated function signature.
internal/exec/describe_config.go (1)
66-67: Injectable writer not used
printOrWriteToFileis already a dependency, but calling it directly prevents mocking.- return d.printOrWriteToFile(d.atmosConfig, format, output, res) + return d.printOrWriteToFile(d.atmosConfig, format, output, res)(No functional change, only clarifies the dependency.)
internal/exec/describe_affected.go (1)
94-103: No test coverage for constructorLines 94-103 are uncovered. A trivial instantiation test would lift coverage and guard against future DI changes.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 94-103: internal/exec/describe_affected.go#L94-L103
Added lines #L94 - L103 were not covered by tests
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)
pkg/utils/highlight_utils.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
pkg/utils/highlight_utils.go
[failure] 139-139:
undefined: HighlightSettings
🪛 GitHub Check: Build (ubuntu-latest, linux)
pkg/utils/highlight_utils.go
[failure] 139-139:
undefined: HighlightSettings
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/utils/highlight_utils.go (4)
33-33: Function signature updated correctly for configuration consistency.The change to accept a pointer to AtmosConfiguration aligns with the broader refactoring for configuration consistency across the codebase. This approach ensures all highlight utility functions have access to the same configuration context.
74-114: Improved modularity through function refactoring.The HighlightCodeWithConfig function has been successfully refactored to extract lexer and formatter selection into helper functions. This addresses the cyclomatic complexity issue flagged in previous reviews and improves code organization.
116-136: Good extraction of lexer selection logic.The helper functions getLexer and isYAML effectively encapsulate the logic for determining the appropriate lexer based on content type. The YAML detection heuristic is simple but sufficient for common YAML formats.
172-172: Correctly updated to use pointer parameter.The call to HighlightCodeWithConfig has been properly updated to pass a pointer to the configuration object, consistent with the function signature change.
|
@coderabbitai please re-review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27166 🏁 Script executed: Length of output: 11572 🏁 Script executed: Length of output: 2856 🏁 Script executed: Length of output: 3784 @samtholiya I've re-reviewed your PR, and it looks good! The implementation of the pager feature for the Key points from my review:
Overall, this is a nice quality-of-life improvement for users when working with large outputs from the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 67176 @samtholiya I've completed a re-review of your PR! The PR is now in a mergeable state and I can see several commits addressing previous feedback. Looking at your recent commits, you've:
The PR looks good overall - you've successfully implemented a terminal pager for the "describe affected" command and related commands, which will significantly improve the user experience when viewing large outputs. The implementation uses Bubble Tea framework appropriately for an interactive terminal experience, and you've provided good test coverage for the new functionality. Some notable strengths:
The PR is now mergeable, and the implementation meets the requirements of adding pager functionality to improve usability for viewing command output content. ( •ᴗ•)⚲ |
samtholiya
force-pushed
the
feature/dev-3182-add-pager-to-atmos-describe-affected-command
branch
from
70871f0 to
215f607
Compare
samtholiya
force-pushed
the
feature/dev-3162-add-pager-to-atmos-describe-component-command
branch
from
85b8fc9 to
0a35636
Compare
Benbentwo
merged commit a0f8b4f
into
feature/dev-3162-add-pager-to-atmos-describe-component-command
Benbentwo
deleted the
feature/dev-3182-add-pager-to-atmos-describe-affected-command
branch
This reverts commit a0f8b4f.
samtholiya
restored the
feature/dev-3182-add-pager-to-atmos-describe-affected-command
branch
|
|
what
why
references
Summary by CodeRabbit
--pagerflag to CLI commands to enable or disable paged output.--pagerflag usage and examples.