Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes multiple help issues #959

Open
wants to merge 78 commits into
base: main
Choose a base branch
from

Conversation

samtholiya
Copy link
Collaborator

@samtholiya samtholiya commented Jan 20, 2025

what

We would be fixing the following UX issues with help in this pr:

  • atmos about non-existent should show usage:
    image

  • Double dash in flags of atmos terraform --help and examples rendering using markdown if available.
    image

  • Fixed atmos workflow --file example.yaml bug for markdown. Now it also exits with exit code 1.
    image

  • Updated Default error logger with markdown support.
    image

  • Added custom alias help support so that alias in config should also be displayed in help.
    image

  • Updated the workflow name invalid UI
    image

  • Invalid custom command config now shows better help
    image

  • Invalid flag usage added
    image

why

  • Outputting markdown in help descriptions makes it easier to visually parse
  • Markdown stylesheet keeps formatting consistent, without each developer needing to know the style guide
  • Changed the way error that exit are displayed, to show more helpful information and usage examples, also formatted in markdown
  • Aliases were not shown in help, making their discoverability difficult

references

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced CLI help outputs and usage examples for core Atmos commands.
    • Added new persistent flags to various commands for improved functionality.
    • Introduced a new command for managing Atlantis configurations and detailed help documentation for it.
    • New command added for validating EditorConfig with comprehensive help documentation.
    • New command for executing Terraform commands within the Atmos framework, including aliases for convenience.
    • New command for displaying information about Atmos with detailed usage instructions.
  • Enhancements

    • Improved error reporting using Markdown-formatted messages that deliver clearer, user-friendly feedback.
    • Updated error messages to use backticks for better readability.
    • Introduced detailed error messages for missing workflows, listing available options.
    • Enhanced error messages for missing Atlantis config templates with structured guidance.
    • Added error messages for invalid commands, specifying the absence of steps or subcommands.
    • Improved error messages for invalid log levels with clearer formatting.
    • Enhanced user guidance for command usage errors, including specific feedback on required subcommands.
    • Streamlined error handling for various commands to provide consistent user feedback.
  • Documentation

    • Updated command usage instructions and examples across multiple areas, including a new "Learn about atmos" section.
    • Added detailed help documentation for new commands and subcommands.
    • Enhanced documentation for Terraform command usage, including specific examples and flags.
  • Refactor

    • Streamlined error handling across the CLI for more consistent and informative responses.
    • Simplified command execution logic by removing unnecessary complexity in error handling.
    • Consolidated error handling methods to enhance clarity and reduce redundancy in the codebase.

@mergify mergify bot added the triage Needs triage label Jan 20, 2025
@samtholiya samtholiya force-pushed the feature/dev-2953-update-help-and-usageyaml-with-snapshots branch from b1741d5 to 01bcd67 Compare January 20, 2025 23:03
@osterman osterman added patch A minor, backward compatible change and removed triage Needs triage labels Jan 22, 2025
@samtholiya samtholiya force-pushed the feature/dev-2953-update-help-and-usageyaml-with-snapshots branch from 018793e to b8a0fb9 Compare January 23, 2025 21:19
@samtholiya samtholiya force-pushed the feature/dev-2953-update-help-and-usageyaml-with-snapshots branch from b8a0fb9 to bb9705b Compare January 23, 2025 21:47
cmd/cmd_utils.go Outdated Show resolved Hide resolved
cmd/list_components.go Outdated Show resolved Hide resolved
@aknysh
Copy link
Member

aknysh commented Feb 5, 2025

@samtholiya please resolve the conflicts and address the comments

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: 0

🧹 Nitpick comments (1)
cmd/cmd_utils.go (1)

121-148: Add validation for empty alias and parts.

The function handles command hierarchy well, but could benefit from additional validation.

Apply this diff to improve input validation:

 func addCommandWithAlias(parentCmd *cobra.Command, alias string, parts []string) {
+    if alias == "" {
+        u.LogErrorAndExit(fmt.Errorf("empty alias provided"))
+    }
     if len(parts) == 0 {
         return
     }
+    if parts[0] == "" {
+        u.LogErrorAndExit(fmt.Errorf("empty command part in alias `%s`", alias))
+    }
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8738804 and 652751b.

📒 Files selected for processing (3)
  • cmd/cmd_utils.go (5 hunks)
  • cmd/terraform.go (2 hunks)
  • internal/exec/vendor_utils.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/exec/vendor_utils.go
  • cmd/terraform.go
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/root.go:172-178
Timestamp: 2025-01-18T15:18:35.475Z
Learning: The `showUsageAndExit` function in `cmd/cmd_utils.go` provides user feedback by showing error messages, command suggestions, and valid subcommands before terminating the program with `os.Exit(1)`. It never returns to the caller, making error handling unnecessary for calls to this function.
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/cmd_utils.go:121-148
Timestamp: 2025-02-03T05:57:18.407Z
Learning: The Atmos CLI should fail fast (exit) when encountering configuration errors, including command alias configuration issues, to prevent undefined behavior. Use LogErrorAndExit instead of returning errors in such cases.
🪛 golangci-lint (1.62.2)
cmd/cmd_utils.go

673-673: printf: non-constant format string in call to fmt.Errorf

(govet)

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

589-674: Well-structured error handling with markdown support!

The error handling implementation is clean and consistent, with good use of markdown for improved readability.

🧰 Tools
🪛 golangci-lint (1.62.2)

673-673: printf: non-constant format string in call to fmt.Errorf

(govet)


673-673: 🛠️ Refactor suggestion

Fix non-constant format string in fmt.Errorf.

The static analysis tool correctly flags the use of a non-constant format string.

Apply this diff to fix the warning:

-    u.PrintErrorMarkdownAndExit("Incorrect Usage", fmt.Errorf(details), suggestion)
+    u.PrintErrorMarkdownAndExit("Incorrect Usage", errors.New(details), suggestion)

Likely invalid or redundant comment.

🧰 Tools
🪛 golangci-lint (1.62.2)

673-673: printf: non-constant format string in call to fmt.Errorf

(govet)

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 5, 2025
@samtholiya samtholiya force-pushed the feature/dev-2953-update-help-and-usageyaml-with-snapshots branch from c38a423 to 316fc49 Compare February 5, 2025 18:47
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: 0

🧹 Nitpick comments (5)
internal/tui/templates/help_printer.go (1)

106-109: LGTM! The condition prevents default values for double dash flags.

The implementation correctly handles the double dash flag case by checking for empty flag names. This aligns with the PR objectives to improve help output clarity.

Consider adding validation for edge cases:

 // if Name is empty it is our double dash.
-if flag.DefValue != "" && flag.Name != "" {
+if flag.DefValue != "" && flag.Name != "" && strings.TrimSpace(flag.Name) != "" {
   description = fmt.Sprintf("%s (default %q)", description, flag.DefValue)
 }
pkg/utils/markdown_utils.go (3)

18-19: Consider concurrency safeguards for the global renderer.
If multiple goroutines invoke these utils simultaneously, consider synchronizing access to the global render to prevent race conditions.


49-65: Return after logging write errors.
Currently, the code logs any error writing to stderr but does not exit or return. Consider returning after logging to avoid proceeding with partial error output.


76-91: Consider return on write failure.
Similar to PrintfErrorMarkdown, returning after an error would prevent further actions when stdout writes fail.

tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden (1)

95-95: Unify dash usage in the example section.
You appear to have used an en dash (–) instead of a plain hyphen (-). Consider standardizing.

-– Execute a terraform
+- Execute a terraform
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 652751b and 7dc5ec7.

📒 Files selected for processing (23)
  • cmd/cmd_utils.go (5 hunks)
  • cmd/list_components.go (1 hunks)
  • cmd/list_workflows.go (1 hunks)
  • cmd/terraform.go (2 hunks)
  • go.mod (1 hunks)
  • internal/tui/templates/help_printer.go (1 hunks)
  • pkg/utils/markdown_utils.go (1 hunks)
  • pkg/version/version.go (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • pkg/version/version.go
🚧 Files skipped from review as they are similar to previous changes (17)
  • tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden
  • cmd/list_workflows.go
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden
  • cmd/terraform.go
  • tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden
  • cmd/list_components.go
  • tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/root.go:172-178
Timestamp: 2025-01-18T15:18:35.475Z
Learning: The `showUsageAndExit` function in `cmd/cmd_utils.go` provides user feedback by showing error messages, command suggestions, and valid subcommands before terminating the program with `os.Exit(1)`. It never returns to the caller, making error handling unnecessary for calls to this function.
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/cmd_utils.go:121-148
Timestamp: 2025-02-03T05:57:18.407Z
Learning: The Atmos CLI should fail fast (exit) when encountering configuration errors, including command alias configuration issues, to prevent undefined behavior. Use LogErrorAndExit instead of returning errors in such cases.
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Summary
🔇 Additional comments (17)
go.mod (1)

61-61: Explicitly requiring golang.org/x/text as a direct dependency.

This change moves the golang.org/x/text v0.21.0 module from an indirect to a direct requirement. It makes it clear that the module is actively used in the project (likely for enhanced markdown rendering and error formatting in light of the PR objectives). Please ensure any related documentation or usage in the code reflects this direct dependency.

pkg/utils/markdown_utils.go (5)

1-2: Documented package purpose well.
Clear and concise introduction to the package's responsibility.


21-47: Handling of stack trace is consistent.
If configured for debug, printing the stack helps with troubleshooting. This logic looks good.


67-70: Simple exit pattern is valid.
Exits immediately after printing error markdown, aligning with fail-fast behavior.


72-74: Invalid usage is handled correctly.
Nicely leverages existing markdown printing for a consistent error message.


93-99: Initialization logic is correct.
Fails fast if markdown renderer cannot be created, ensuring no half-initialized state.

tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden (4)

2-3: Usage description is clear.
Gives context about Terraform commands and how they integrate with Atmos.


6-48: Command references look consistent.
All sub-commands and native Terraform commands are correctly listed.


51-63: Flags documentation is thorough.
Properly explains how users can customize behavior.


73-80: Global flags are detailed.
Well-described logging and redirection settings.

cmd/cmd_utils.go (7)

49-53: Alias mapping looks good.
The iteration over command aliases is concise and straightforward.


121-148: Fail-fast approach is consistent with prior learnings.
This function enforces correctness by exiting when a subcommand is unavailable, which prevents unexpected behavior.


224-226: Clear handling of invalid command scenario.
Exits with Markdown-based error, ensuring user feedback.


590-599: Usage fallback is user-friendly.
Graceful fallback to show usage or relevant subcommands is well-executed.


601-614: Flag error usage pathway is consistent.
Generates a descriptive error and exits. This matches the rest of the CLI's fail-fast behavior.


637-664: Helpful subcommand suggestions.
Logic for displaying valid subcommands or suggestions is robust.


666-674: Markdown usage examples are integrated strongly.
Provides a clear fallback and custom usage examples if available.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 5, 2025
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: 1

🧹 Nitpick comments (2)
cmd/cmd_utils.go (1)

224-226: Simplify error creation using fmt.Errorf.

The current error creation can be simplified for better readability.

Apply this diff:

-			u.PrintErrorMarkdownAndExit("Invalid command", errors.New(
-				fmt.Sprintf("no args, no steps, no sub-commands provided in config for `%s`", cmd.CommandPath()),
-			), "https://atmos.tools/cli/configuration/commands")
+			u.PrintErrorMarkdownAndExit("Invalid command", 
+				fmt.Errorf("no args, no steps, no sub-commands provided in config for `%s`", cmd.CommandPath()),
+				"https://atmos.tools/cli/configuration/commands")
🧰 Tools
🪛 golangci-lint (1.62.2)

224-224: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))

(gosimple)

pkg/ui/markdown/renderer.go (1)

137-137: Simplify string formatting.

The fmt.Sprintf is unnecessary when the argument is already a string.

-		content += fmt.Sprintf("%s", details)
+		content += details
🧰 Tools
🪛 golangci-lint (1.62.2)

137-137: S1025: the argument is already a string, there's no need to use fmt.Sprintf

(gosimple)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7dc5ec7 and d3790d0.

📒 Files selected for processing (2)
  • cmd/cmd_utils.go (5 hunks)
  • pkg/ui/markdown/renderer.go (5 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/root.go:172-178
Timestamp: 2025-01-18T15:18:35.475Z
Learning: The `showUsageAndExit` function in `cmd/cmd_utils.go` provides user feedback by showing error messages, command suggestions, and valid subcommands before terminating the program with `os.Exit(1)`. It never returns to the caller, making error handling unnecessary for calls to this function.
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/cmd_utils.go:121-148
Timestamp: 2025-02-03T05:57:18.407Z
Learning: The Atmos CLI should fail fast (exit) when encountering configuration errors, including command alias configuration issues, to prevent undefined behavior. Use LogErrorAndExit instead of returning errors in such cases.
🪛 golangci-lint (1.62.2)
pkg/ui/markdown/renderer.go

20-20: field atmosConfig is unused

(unused)


137-137: S1025: the argument is already a string, there's no need to use fmt.Sprintf

(gosimple)

cmd/cmd_utils.go

224-224: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))

(gosimple)

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

121-148: LGTM! Well-structured command alias handling.

The recursive implementation for building command hierarchies is clean and efficient. The early return and proper error handling align with the CLI's fail-fast philosophy.


589-674: LGTM! User-friendly help and usage display.

The implementation provides excellent user experience with:

  • Markdown-formatted usage examples
  • Command suggestions
  • Clear subcommand listings
pkg/ui/markdown/renderer.go (2)

16-21: LGTM! Constructor changes look good.

The addition of atmosConfig and its integration into the constructor enhances the renderer's configurability.

Also applies to: 23-55

🧰 Tools
🪛 golangci-lint (1.62.2)

20-20: field atmosConfig is unused

(unused)


180-195: LGTM! Robust terminal width handling.

The implementation properly handles type assertion failures and provides appropriate fallbacks, addressing previous concerns about nil pointer issues.

pkg/ui/markdown/renderer.go Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 5, 2025
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: 0

🧹 Nitpick comments (3)
tests/test-cases/complete.yaml (1)

18-35: Clarify Test Case Description for atmos workflow pla --file workflow1

The new test case is well structured and correctly captures the expected output (error messages and exit code 1). However, the description ("Ensure atmos workflow pla --file workflow1 runs successfully") might be a bit misleading since the command is intended to trigger an error state (exit code 1). Consider updating the description to reflect that the command is handling a missing workflow scenario.

tests/fixtures/scenarios/complete/atmos.yaml (1)

246-248: Refine Invalid Command Configuration

A new command entry for invalidCommand is added. The configuration correctly omits any steps or subcommands, which will trigger the enhanced error messaging. For improved clarity and grammar, consider rephrasing the description to:
"This is an invalid command that has neither steps nor a subcommand configured"
This minor change will improve readability.

cmd/cmd_utils.go (1)

224-226: Simplify error creation using fmt.Errorf.

The error creation can be simplified by using fmt.Errorf directly instead of wrapping fmt.Sprintf with errors.New.

Apply this diff to simplify the error creation:

-			u.PrintErrorMarkdownAndExit("Invalid command", errors.New(
-				fmt.Sprintf("The `%s` command has no steps or subcommands configured.", cmd.CommandPath()),
-			), "https://atmos.tools/cli/configuration/commands")
+			u.PrintErrorMarkdownAndExit("Invalid command", 
+				fmt.Errorf("The `%s` command has no steps or subcommands configured.", cmd.CommandPath()),
+				"https://atmos.tools/cli/configuration/commands")
🧰 Tools
🪛 golangci-lint (1.62.2)

224-224: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))

(gosimple)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d3790d0 and ffc48a3.

📒 Files selected for processing (4)
  • cmd/cmd_utils.go (5 hunks)
  • tests/fixtures/scenarios/complete/atmos.yaml (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_invalidCommand.stderr.golden (1 hunks)
  • tests/test-cases/complete.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • tests/snapshots/TestCLICommands_atmos_invalidCommand.stderr.golden
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/root.go:172-178
Timestamp: 2025-01-18T15:18:35.475Z
Learning: The `showUsageAndExit` function in `cmd/cmd_utils.go` provides user feedback by showing error messages, command suggestions, and valid subcommands before terminating the program with `os.Exit(1)`. It never returns to the caller, making error handling unnecessary for calls to this function.
Learnt from: samtholiya
PR: cloudposse/atmos#959
File: cmd/cmd_utils.go:121-148
Timestamp: 2025-02-03T05:57:18.407Z
Learning: The Atmos CLI should fail fast (exit) when encountering configuration errors, including command alias configuration issues, to prevent undefined behavior. Use LogErrorAndExit instead of returning errors in such cases.
🪛 golangci-lint (1.62.2)
cmd/cmd_utils.go

224-224: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...))

(gosimple)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Summary
🔇 Additional comments (3)
tests/test-cases/complete.yaml (1)

36-48: Validate Invalid Command Test Case

The test case for atmos invalidCommand clearly verifies that an unknown command returns the expected error message ("command has no steps or subcommands configured") and exit code 1. This aligns with the updated error handling logic in the CLI.

cmd/cmd_utils.go (2)

49-53: Well-structured command alias implementation!

The command alias handling is implemented with proper validation and follows the fail-fast principle for configuration errors. The recursive approach for building command hierarchies is clean and maintainable.

Also applies to: 121-148


589-674: Excellent user-friendly error handling!

The implementation provides a great user experience with:

  • Intelligent command suggestions
  • Clear listing of valid subcommands
  • Markdown-formatted usage examples
  • Contextual error messages

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 5, 2025
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: 0

🧹 Nitpick comments (1)
cmd/root.go (1)

21-22: Consider consolidating utils imports.

You have two imports of the same package:

"github.com/cloudposse/atmos/pkg/utils"
u "github.com/cloudposse/atmos/pkg/utils"

Consider using just one import to reduce confusion:

-"github.com/cloudposse/atmos/pkg/utils"
-u "github.com/cloudposse/atmos/pkg/utils"
+u "github.com/cloudposse/atmos/pkg/utils"
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between ffc48a3 and e71cb8d.

📒 Files selected for processing (5)
  • cmd/root.go (4 hunks)
  • tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (3 hunks)
  • tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden (2 hunks)
  • tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden (2 hunks)
  • tests/test-cases/log-level-validation.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_--help.stdout.golden
  • tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden
  • tests/test-cases/log-level-validation.yaml
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
  • GitHub Check: Acceptance Tests (macos-latest, macos)
  • GitHub Check: Acceptance Tests (windows-latest, windows)
  • GitHub Check: Acceptance Tests (ubuntu-latest, linux)
  • GitHub Check: [localstack] demo-localstack
  • GitHub Check: Summary
🔇 Additional comments (3)
cmd/root.go (3)

79-79: LGTM! Consistent error handling with markdown support.

The change from LogErrorAndExit to PrintErrorMarkdownAndExit aligns with the PR objective of standardizing error handling with markdown support.


138-138: LGTM! Markdown initialization added.

The addition of utils.InitializeMarkdown(atmosConfig) supports the PR objective of improving help output with markdown rendering.


215-217: LGTM! Enhanced flag error handling.

The new SetFlagErrorFunc implementation improves user guidance for invalid flag usage, which aligns with the PR objectives.

@aknysh
Copy link
Member

aknysh commented Feb 6, 2025

@samtholiya please address the comment.

Also, please update the ##what and ##why in the PR description to better explain what issues are we fixing (or we adding new features?), and why, instead of this:

We would be fixing multiple UX issues with help in this pr

It's a big PR, and just saying "We would be fixing multiple UX issues" does not help to understand what is being fixed and why.

When we create a new release, we could not just say "We would be fixing multiple UX issues with help in this release", we would need a better ##what was done and ##why

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: 0

🧹 Nitpick comments (1)
cmd/root.go (1)

21-22: Consider consolidating duplicate package imports.

You're importing the same package twice with different aliases. Consider using just one import with the alias u since it's already being used throughout the code.

-	"github.com/cloudposse/atmos/pkg/utils"
	u "github.com/cloudposse/atmos/pkg/utils"
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between e71cb8d and d4a5a5c.

📒 Files selected for processing (3)
  • cmd/root.go (4 hunks)
  • go.mod (1 hunks)
  • tests/test-cases/log-level-validation.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test-cases/log-level-validation.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Summary
🔇 Additional comments (4)
go.mod (1)

61-61: Direct Dependency on golang.org/x/text Established.
This change removes the "indirect" annotation, making golang.org/x/text a direct dependency. This is likely necessary to support the updated markdown rendering in help and error messages. Please ensure that all module usages related to text processing have been aligned with this update and verify there are no dependency conflicts.

cmd/root.go (3)

81-81: LGTM! Enhanced error handling with markdown support.

The change from LogErrorAndExit to PrintErrorMarkdownAndExit aligns with the PR's objective to improve error message readability.


268-270: LGTM! Improved flag error handling.

The addition of a dedicated flag error handler using showFlagUsageAndExit enhances the user experience by providing better guidance for invalid flag usage.


176-176: Verify markdown initialization timing.

The markdown initialization is placed after error checking. Please ensure that any code paths that might use markdown features before this point are properly handled.

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Search for markdown-related function calls that might execute before initialization
rg -p "PrintErrorMarkdown|RenderMarkdown" | grep -B 5 "InitializeMarkdown" || echo "No early markdown usage found"

Length of output: 143


Markdown Initialization Timing Verified

The shell script search confirms there are no markdown function calls (such as PrintErrorMarkdown or RenderMarkdown) occurring before the call to utils.InitializeMarkdown. Therefore, the initialization appears to be correctly placed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants