Skip to content

Conversation

samtholiya
Copy link
Collaborator

@samtholiya samtholiya commented Apr 23, 2025

what

  • We are adding pager to atmos describe component
  • We fixed the bug in the function that would assume the data is always atmosConfig and then use it to Highlight the data. This was all good if the data is atmosConfig but not for other cases.

image

why

  • We want user to give a consistent experience.

references

Summary by CodeRabbit

  • New Features
    • Introduced a pager option for the describe component command, enabling paged output with syntax highlighting for YAML and JSON formats.
    • Enhanced output formatting with customizable syntax highlighting based on user configuration.
  • Bug Fixes
    • Improved argument validation and error handling for CLI commands.
  • Documentation
    • Added documentation for the new --pager flag in the describe component command.
  • Tests
    • Added unit and integration tests covering pager functionality, output formatting, and CLI command execution.
  • Refactor
    • Refactored command execution and output handling for improved modularity, testability, and configuration management.
    • Updated output functions to accept configuration context for consistent formatting and highlighting.

samtholiya and others added 30 commits April 14, 2025 23:21
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (3)
cmd/describe_component.go (1)

82-88: Consider less extreme error handling than panic.

While the comment explains the rationale, panic is a severe response that terminates the application. Consider using a more graceful error handling approach like logging the error and exiting with a non-zero status code.

-// We prefer to panic because this is a developer error.
-// checkFlagNotPresentError checks if the error is nil.
-func checkFlagNotPresentError(err error) {
-	if err != nil {
-		panic(err)
-	}
-}
+// checkFlagNotPresentError handles errors from flag retrieval.
+// These errors should never happen unless there's a developer error.
+func checkFlagNotPresentError(err error) {
+	if err != nil {
+		u.LogErrorAndExit(err)
+	}
+}
cmd/describe_affected.go (1)

142-147: Cyclomatic complexity nudged over the threshold

parseDescribeAffectedCliArgs hit a value of 11 (>10).
After fixing the panic above, splitting flag parsing into its own helper would drop the count and keep the function easier to reason about.
Not urgent, but worth tidying.

internal/exec/describe_affected.go (1)

107-114: Heavy value parameter – pass by pointer

Execute(a DescribeAffectedCmdArgs) copies a ~6 KB struct twice (caller + callee).
Switch the signature to Execute(a *DescribeAffectedCmdArgs) (and update call-sites) to avoid the copy and quiet the linter.

🧰 Tools
🪛 GitHub Check: golangci-lint

[failure] 107-107:
hugeParam: a is heavy (6120 bytes); consider passing it by pointer

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ac04fc and a0f8b4f.

📒 Files selected for processing (6)
  • cmd/describe_affected.go (3 hunks)
  • cmd/describe_component.go (2 hunks)
  • internal/exec/describe_affected.go (5 hunks)
  • internal/exec/describe_affected_test.go (1 hunks)
  • internal/exec/describe_component_test.go (1 hunks)
  • pkg/utils/highlight_utils.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/exec/describe_component_test.go
  • pkg/utils/highlight_utils.go
🧰 Additional context used
🪛 GitHub Check: golangci-lint
cmd/describe_affected.go

[failure] 65-65:
cyclomatic: function parseDescribeAffectedCliArgs has cyclomatic complexity 11 (> max enabled 10)

internal/exec/describe_affected.go

[failure] 107-107:
hugeParam: a is heavy (6120 bytes); consider passing it by pointer


[failure] 168-168:
hugeParam: a is heavy (6120 bytes); consider passing it by pointer

cmd/describe_component.go

[failure] 12-12:
var errInvalidFlag is unused

🪛 golangci-lint (1.64.8)
internal/exec/describe_affected.go

[warning] 169-169: if a.Query == "" has complex nested blocks (complexity: 4)

(nestif)

cmd/describe_component.go

[error] 12-12: var errInvalidFlag is unused

(unused)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Summary
🔇 Additional comments (4)
cmd/describe_component.go (4)

41-42: Great addition of pager flag!

This matches the PR objective to implement a native pager for describe commands. The implementation properly retrieves and passes the pager setting.


46-56: Clean refactoring to structured parameters.

The move to DescribeComponentParams struct makes the code more maintainable and easier to test. This is a solid improvement over passing individual arguments.


68-68: Good simplification of format flag description.

The simplified description is more concise while remaining clear.


20-20: Good use of ExactArgs for validation.

Using cobra.ExactArgs(1) is a solid improvement that ensures the command receives exactly one component argument.

Copy link

mergify bot commented May 15, 2025

💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏

@mergify mergify bot added conflict This PR has conflicts and removed conflict This PR has conflicts labels May 15, 2025
@samtholiya samtholiya force-pushed the feature/dev-3162-add-pager-to-atmos-describe-component-command branch from b6cdfa5 to 1ac04fc Compare May 15, 2025 21:44
Copy link

mergify bot commented May 15, 2025

💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏

@mergify mergify bot added the conflict This PR has conflicts label May 15, 2025
@samtholiya samtholiya force-pushed the feature/dev-3162-add-pager-to-atmos-describe-component-command branch from 1ac04fc to 0a35636 Compare May 15, 2025 21:48
Copy link
Member

@Benbentwo Benbentwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why the pager flag isn't a bool? I'm not following why we use certain strings.

@Benbentwo Benbentwo merged commit 602f8f3 into main May 16, 2025
51 checks passed
@Benbentwo Benbentwo deleted the feature/dev-3162-add-pager-to-atmos-describe-component-command branch May 16, 2025 12:32
@mergify mergify bot removed the needs-cloudposse Needs Cloud Posse assistance label May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants