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

Standardize Help and Usage content for all commands and sub commands #914

Merged
merged 78 commits into from
Jan 18, 2025

Conversation

samtholiya
Copy link
Collaborator

@samtholiya samtholiya commented Jan 5, 2025

what

  • Every command will have intutively display help and usage
    Example usage scenarios:
    image
    Example help scenarios:
    image
    image
    image

Example atmos config error code
image

Example alias subCommand section
image

why

  • Help and usage displays were not consistently handled across multiple commands

references

Summary by CodeRabbit

Release Notes

Command Line Interface Improvements

  • Enhanced command argument validation across multiple commands, enforcing stricter command usage.
  • Added robust help request handling for various commands.
  • Improved error messaging for unknown commands and streamlined command execution processes.

New Features

  • Introduced new Helmfile sub-commands:
    • helmfile apply
    • helmfile destroy
    • helmfile diff
    • helmfile sync
  • Added custom colored command-line output support.
  • Introduced a new function for printing error messages in color.

Command Behavior Changes

  • Most commands now explicitly reject additional arguments.
  • Simplified help and usage display mechanisms.
  • Removed automatic help information for empty subcommands.
  • Updated command usage syntax to emphasize sub-commands.
  • Introduced sub-command aliases for improved usability.

Dependency Updates

  • Removed coloredcobra library.
  • Added golang.org/x/oauth2 dependency.

Usability Enhancements

  • More consistent command-line interface with improved error handling and messaging.
  • Enhanced template generation for command help.
  • Added support for displaying subcommand aliases in help templates.

dev-2821-atmos-terraform-help-should-also-show-terraform-help
…terraform-help' of https://github.com/cloudposse/atmos into feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
…terraform-help' of https://github.com/cloudposse/atmos into feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
@samtholiya samtholiya changed the base branch from main to feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help January 5, 2025 19:29
@mergify mergify bot added the triage Needs triage label Jan 5, 2025
Copy link

mergify bot commented Jan 5, 2025

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

@mergify mergify bot added the conflict This PR has conflicts label Jan 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: 4

🧹 Nitpick comments (2)
tests/test-cases/help-and-usage.yaml (2)

1-57: Consider strengthening the validation patterns.

The test cases look solid! To make them even more robust, consider:

  1. Using more specific regex patterns (e.g., ^Usage: instead of just Usage:)
  2. Validating the complete error message format
 expect:
   stderr:
-    - 'Error: Unknown command \"non-existent\" for \"atmos\"'
+    - '^Error: Unknown command \"non-existent\" for \"atmos\"\s*$'

236-236: Remove trailing spaces.

Remove trailing spaces from these lines to fix the YAML linting errors.

Also applies to: 250-250, 264-264

🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between f1e2b9b and 3637ae4.

📒 Files selected for processing (2)
  • cmd/atlantis_generate_repo_config.go (1 hunks)
  • tests/test-cases/help-and-usage.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/atlantis_generate_repo_config.go
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/test-cases/help-and-usage.yaml

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)


[error] 264-264: trailing spaces

(trailing-spaces)


[error] 447-447: duplication of key "stderr" in mapping

(key-duplicates)


[error] 446-446: syntax error: expected , but found ''

(syntax)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Summary

tests/test-cases/help-and-usage.yaml Outdated Show resolved Hide resolved
tests/test-cases/help-and-usage.yaml Outdated Show resolved Hide resolved
tests/test-cases/help-and-usage.yaml Outdated Show resolved Hide resolved
tests/test-cases/help-and-usage.yaml Outdated Show resolved Hide resolved
@samtholiya samtholiya force-pushed the feature/dev-2896-incorrect-output-for-atmos-validate branch from 3637ae4 to fe0706c Compare January 16, 2025 00:40
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

♻️ Duplicate comments (2)
tests/test-cases/help-and-usage.yaml (2)

117-117: ⚠️ Potential issue

Fix the truncated help text.

The help text appears to be cut off mid-sentence: "and the beginning of additional native arguments and flags for the specific command bei". This needs to be completed for proper validation.

Also applies to: 134-134


250-263: ⚠️ Potential issue

Remove duplicate test case.

This test case is identical to the one at lines 236-249. Remove the duplicate to maintain test clarity and reduce maintenance overhead.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 250-250: trailing spaces

(trailing-spaces)

🧹 Nitpick comments (2)
tests/test-cases/help-and-usage.yaml (2)

236-236: Clean up trailing whitespace.

Remove trailing spaces from these lines to maintain consistent formatting.

Also applies to: 250-250, 264-264

🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)


1-1: Add test cases for command suggestion behavior.

Based on the PR discussion about cobra's behavior, consider adding test cases that verify the command suggestion feature when typos are made. For example:

  - name: atmos terrafom
    enabled: true
    description: "Should suggest correct command for typo"
    workdir: "../"
    command: "atmos"
    args:
      - "terrafom"
    expect:
      stderr:
        - 'Error: unknown command "terrafom" for "atmos"'
        - 'Did you mean this?'
        - '\s+terraform'
      exit_code: 1
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3637ae4 and fe0706c.

📒 Files selected for processing (2)
  • cmd/atlantis_generate_repo_config.go (1 hunks)
  • tests/test-cases/help-and-usage.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/atlantis_generate_repo_config.go
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/test-cases/help-and-usage.yaml

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)


[error] 264-264: trailing spaces

(trailing-spaces)


[error] 447-447: duplication of key "stderr" in mapping

(key-duplicates)

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

1-57: Well-structured basic command test cases!

The test cases effectively validate core CLI behaviors including error handling, help output, and basic command functionality.


445-449: ⚠️ Potential issue

Fix YAML syntax and logic errors in the last test case.

There are several issues in this test case:

  1. The stderr key is duplicated
  2. The exit code should be 1 for an error condition, not 0

Apply this fix:

  expect:
    stderr:
      - 'Error: Unknown command "non-existant" for "atmos atlantis generate repo-config"'
-    stderr:
-      - "^$"
-    exit_code: 0
+    exit_code: 1

Likely invalid or redundant comment.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 447-447: duplication of key "stderr" in mapping

(key-duplicates)

@samtholiya samtholiya force-pushed the feature/dev-2896-incorrect-output-for-atmos-validate branch from fe0706c to 9cf96ce Compare January 16, 2025 01:02
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 (4)
tests/test-cases/help-and-usage.yaml (4)

41-41: Enhance readability of usage pattern.

The usage pattern could be more readable with proper formatting and clearer placeholders.

-        - "\\batmos \\[subcommand\\] [<]component[>] -s [<]stack[>] -- [<]native-flags[>]"
+        - "\\batmos \\[subcommand\\] <component> -s <stack> -- <native-flags>"

236-236: Remove trailing spaces in test names.

Remove trailing spaces after "atmos helmfile apply" in test names.

-  - name: atmos helmfile apply 
+  - name: atmos helmfile apply

Also applies to: 250-250, 264-264

🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)


434-451: Add test cases for command suggestion behavior.

Based on the PR discussion about cobra's behavior, consider adding test cases that verify the command suggestion feature when typos are made.

Example test case:

  - name: atmos terrafom
    enabled: true
    description: "Should suggest correct command for typo"
    workdir: "../"
    command: "atmos"
    args:
      - "terrafom"
    expect:
      stderr:
        - 'Error: unknown command "terrafom" for "atmos"'
        - 'Did you mean this?'
        - '\s+terraform'
      exit_code: 1

1-451: Consider restructuring the test suite for better maintainability.

The test suite is comprehensive but could benefit from:

  1. Grouping similar test cases using YAML anchors to reduce duplication
  2. Adding comments to separate major command sections
  3. Consistent naming conventions for test cases (e.g., "Should show..." vs "Ensure...")

Example structure:

# Common test patterns
common_patterns: &common_patterns
  enabled: true
  workdir: "../"
  command: "atmos"

# Basic command tests
tests:
  - name: atmos non-existent
    <<: *common_patterns
    description: "Should show error for non-existent command"
    ...
🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)


[error] 264-264: trailing spaces

(trailing-spaces)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between fe0706c and 9cf96ce.

📒 Files selected for processing (2)
  • cmd/atlantis_generate_repo_config.go (1 hunks)
  • tests/test-cases/help-and-usage.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/atlantis_generate_repo_config.go
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/test-cases/help-and-usage.yaml

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)


[error] 264-264: trailing spaces

(trailing-spaces)

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

117-117: Complete the truncated help text.

The help text appears to be cut off mid-sentence: "and the beginning of additional native arguments and flags for the specific command bei". Please include the complete message.

Also applies to: 134-134


250-263: Remove duplicate test case.

This test case is identical to the one at lines 236-249.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 250-250: trailing spaces

(trailing-spaces)

@samtholiya samtholiya force-pushed the feature/dev-2896-incorrect-output-for-atmos-validate branch from 004ea1c to c3a6b6e Compare January 16, 2025 02:28
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

♻️ Duplicate comments (2)
tests/test-cases/help-and-usage.yaml (2)

117-117: ⚠️ Potential issue

Fix the truncated help text.

The help text appears to be cut off mid-sentence: "and the beginning of additional native arguments and flags for the specific command bei". Please provide the complete help text.

Also applies to: 134-134


236-263: ⚠️ Potential issue

Remove duplicate test case.

The test case "atmos helmfile apply" is duplicated. Remove one instance to maintain test clarity.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)

🧹 Nitpick comments (3)
tests/test-cases/help-and-usage.yaml (3)

4-4: Enhance test descriptions for clarity.

The test descriptions could be more specific about what they're validating:

  • Line 4: Consider "Verify error handling when executing a non-existent command"
  • Line 15: Consider "Verify that '--help' flag displays all available commands and their descriptions"
  • Line 48: Consider "Verify behavior when running atmos without a config file"

Also applies to: 15-15, 48-48


236-236: Fix trailing spaces.

Remove trailing spaces from the following lines:

  • Line 236: atmos helmfile apply
  • Line 250: atmos helmfile apply
  • Line 264: atmos helmfile apply

Also applies to: 250-250, 264-264

🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)


1-447: Standardize error message validation across test cases.

Consider standardizing how error messages are validated:

  1. Some test cases use exact matches while others use regex patterns
  2. Some error messages are in stdout while others are in stderr
  3. Some test cases validate empty stderr (^$) while others don't

This standardization would improve maintainability and make the test suite more robust.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)


[error] 264-264: trailing spaces

(trailing-spaces)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9cf96ce and f504b75.

📒 Files selected for processing (2)
  • cmd/atlantis_generate_repo_config.go (1 hunks)
  • tests/test-cases/help-and-usage.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/atlantis_generate_repo_config.go
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/test-cases/help-and-usage.yaml

[error] 236-236: trailing spaces

(trailing-spaces)


[error] 250-250: trailing spaces

(trailing-spaces)


[error] 264-264: trailing spaces

(trailing-spaces)

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: [localstack] demo-localstack
  • GitHub Check: Acceptance Tests (macos-latest, macos)
  • GitHub Check: Acceptance Tests (windows-latest, windows)
  • GitHub Check: Acceptance Tests (ubuntu-latest, linux)
  • GitHub Check: Summary

tests/test-cases/help-and-usage.yaml Show resolved Hide resolved
@osterman osterman added minor New features that do not break anything and removed patch A minor, backward compatible change labels Jan 16, 2025
Copy link

mergify bot commented Jan 16, 2025

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

@mergify mergify bot added the conflict This PR has conflicts label Jan 16, 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

♻️ Duplicate comments (1)
tests/test-cases/help-and-usage.yaml (1)

117-117: ⚠️ Potential issue

Complete the truncated help text.

The help text appears to be truncated. Based on the context, this should include the complete message about native arguments and flags.

-        - "and the beginning of additional native arguments and flags for the specific command being run"
+        - "and the beginning of additional native arguments and flags for the specific command being executed"
🧹 Nitpick comments (5)
tests/test-cases/help-and-usage.yaml (5)

41-41: Simplify the regex pattern for command usage.

The pattern \\batmos \\[subcommand\\] [<]component[>] -s [<]stack[>] -- [<]native-flags[>] escapes angle brackets, which makes it harder to read. Consider using a simpler pattern that matches the actual output format.

-        - "\\batmos \\[subcommand\\] [<]component[>] -s [<]stack[>] -- [<]native-flags[>]"
+        - "\\batmos \\[subcommand\\] <component> -s <stack> -- <native-flags>"

48-48: Complete the description text.

The description appears to be truncated and missing "file was not found" at the end.

-    description: "Ensure atmos CLI help command shows atmos config "
+    description: "Ensure atmos CLI help command shows atmos config file was not found"

123-123: Fix incorrect test description.

The description doesn't match the actual test case which is specifically for "terraform apply help".

-    description: "Ensure atmos terraform help shows help output"
+    description: "Ensure atmos terraform apply help shows help output"

250-250: Remove trailing whitespace.

There is trailing whitespace in the test name.

-  - name: atmos helmfile apply 
+  - name: atmos helmfile apply
🧰 Tools
🪛 yamllint (1.35.1)

[error] 250-250: trailing spaces

(trailing-spaces)


193-193: Fix typo in test name and description.

The word "non-existant" is misspelled. It should be "non-existent".

-  - name: atmos helmfile non-existant
-    description: "Should show usage for helmfile and non-existant subcommand"
+  - name: atmos helmfile non-existent
+    description: "Should show usage for helmfile and non-existent subcommand"

Also applies to: 194-194

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between f504b75 and c7ee7e7.

📒 Files selected for processing (1)
  • tests/test-cases/help-and-usage.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
tests/test-cases/help-and-usage.yaml

[error] 250-250: trailing spaces

(trailing-spaces)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (1)
tests/test-cases/help-and-usage.yaml (1)

420-433: Add test cases for command suggestion behavior.

Based on the PR discussion about cobra's behavior, we should add test cases that verify the command suggestion feature when typos are made.

Add the following test case:

  - name: atmos terrafom
    enabled: true
    description: "Verify command suggestion for typo"
    workdir: "../"
    command: "atmos"
    args:
      - "terrafom"
    expect:
      stderr:
        - 'Error: unknown command "terrafom" for "atmos"'
        - 'Did you mean this?'
        - '\s+terraform'
      exit_code: 1

@aknysh
Copy link
Member

aknysh commented Jan 17, 2025

@samtholiya please resolve the conflicts

@mergify mergify bot removed the conflict This PR has conflicts label Jan 18, 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: 3

🧹 Nitpick comments (3)
cmd/root.go (2)

133-145: Consider adding input validation.

The function handles the happy path well, but could benefit from input validation.

Consider this enhancement:

 func getInvalidCommandName(input string) string {
+  if input == "" {
+    return ""
+  }
   // Regular expression to match the command name inside quotes
   re := regexp.MustCompile(`unknown command "([^"]+)"`)

Line range hint 179-207: Consider simplifying help function logic.

The help function handles errors well but the logic flow could be more straightforward.

Consider extracting the logo printing logic into a separate function to reduce duplication:

+func printAtmosLogo() error {
+  fmt.Println()
+  return tuiUtils.PrintStyledText("ATMOS")
+}
+
 RootCmd.SetHelpFunc(func(command *cobra.Command, args []string) {
   // ... existing conditions ...
-  fmt.Println()
-  err := tuiUtils.PrintStyledText("ATMOS")
+  err := printAtmosLogo()
   if err != nil {
     u.LogErrorAndExit(atmosConfig, err)
   }
   // ... rest of the function ...
cmd/terraform.go (1)

39-41: Consider using handleHelpRequest for consistent help handling.

Since this PR aims to standardize help and usage content, consider using the new handleHelpRequest utility function mentioned in the PR summary.

 	if info.NeedHelp {
-		actualCmd.Usage()
-		return
+		return handleHelpRequest(actualCmd)
 	}
🧰 Tools
🪛 golangci-lint (1.62.2)

40-40: Error return value of actualCmd.Usage is not checked

(errcheck)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between c7ee7e7 and ac22b72.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (9)
  • cmd/root.go (6 hunks)
  • cmd/terraform.go (3 hunks)
  • cmd/version.go (1 hunks)
  • go.mod (0 hunks)
  • internal/tui/templates/templater.go (5 hunks)
  • pkg/utils/log_utils.go (1 hunks)
  • tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden (3 hunks)
  • tests/test-cases/core.yaml (1 hunks)
  • tests/test-cases/empty-dir.yaml (2 hunks)
💤 Files with no reviewable changes (1)
  • go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
  • cmd/version.go
  • tests/test-cases/core.yaml
  • pkg/utils/log_utils.go
  • internal/tui/templates/templater.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/terraform.go

40-40: Error return value of actualCmd.Usage is not checked

(errcheck)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Summary
🔇 Additional comments (7)
tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden (2)

28-31: Well done on adding command aliases!

The addition of aliases hf and tf for commonly used commands will improve developer productivity. This is a great quality-of-life enhancement.


6-6: Strong work on standardizing command terminology!

The consistent use of [sub-command] notation and updated example usage improves clarity. This aligns well with the PR's objective of standardizing help content.

Also applies to: 59-59, 62-62

tests/test-cases/empty-dir.yaml (2)

31-31: Good catch on the exit code!

Changing the exit code to 1 when config is missing is more appropriate. This follows the Unix convention where non-zero exit codes indicate errors.


45-45: Consistent terminology in test expectations.

The updated help text matches the new standardized terminology using "subcommand". This maintains consistency across the codebase.

cmd/root.go (1)

7-8: LGTM! Good additions for enhanced functionality.

The new imports and FParseErrWhitelist field are well-chosen for improving command parsing flexibility and error handling.

Also applies to: 13-13, 30-30

cmd/terraform.go (2)

7-7: LGTM! Clean import addition.

The cobra import is properly formatted and essential for the CLI functionality.


22-24: Verify hook execution with the new config info retrieval!

The refactoring looks good! The use of getConfigAndStacksInfo simplifies the implementation while maintaining hook functionality.

Let's verify the hook execution still works as expected:

✅ Verification successful

Hook execution verified - Implementation is battle-ready! 💪

The refactoring maintains a solid execution flow:

  • getConfigAndStacksInfo properly processes command arguments and generates the required configuration
  • hooks.RunE correctly receives and handles the configuration info
  • Error handling is robust throughout the execution chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if hooks are properly registered and called
rg -A 5 "RunE.*info" pkg/hooks/

Length of output: 357


Script:

#!/bin/bash
# Check getConfigAndStacksInfo implementation
ast-grep --pattern 'func getConfigAndStacksInfo($$$) $$$'

# Look for hook-related code and tests
rg -g '!vendor' -A 5 'hooks\.' cmd/
rg -g '!vendor' -l 'hooks.*test' .

Length of output: 1409

cmd/root.go Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
cmd/terraform.go Show resolved Hide resolved
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

🔭 Outside diff range comments (1)
tests/test-cases/log-level-validation.yaml (1)

Warrior, your instincts serve you well. These test changes indeed stray from our mission.

The log level validation tests appear to be unrelated to the PR's objective of standardizing help and usage content. The code search reveals that while log level validation is an existing feature in the codebase (in pkg/config/), these test additions are not connected to any help/usage content standardization efforts.

🔗 Analysis chain

Line range hint 1-80: Verify alignment with PR objectives.

Spartan, these test changes seem unrelated to the PR's objective of standardizing help and usage content. The modifications to log level validation behavior appear to be outside the scope of the current changes.

Let me check if there are related changes in other files:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related changes in help/usage standardization
rg -l "cobra.NoArgs|handleHelpRequest|showUsageAndExit" --type go

# Check if log level validation is modified elsewhere
git diff | grep -A 5 "logs-level"

Length of output: 904


Script:

#!/bin/bash
# Search for log level validation code
ast-grep --pattern 'Invalid log level.*Valid options'

# Search for log level related code
rg "ATMOS_LOGS_LEVEL|--logs-level" --type go -A 3

# Check for other test changes in the same PR
git diff -- 'tests/**/*.yaml'

Length of output: 1298


Script:

#!/bin/bash
# Search for help/usage message changes
git diff -U0 | rg "Usage:|Error:" -B2 -A2

# Check for cobra command changes related to error messages
rg "cobra.*Error" --type go -A 3

Length of output: 78

🧹 Nitpick comments (1)
tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (1)

Line range hint 1-62: Consider adding examples for the new aliases.

While the aliases are well-documented, consider enhancing the example section to demonstrate their usage. This would help users quickly understand how to leverage these shortcuts.

Add examples like:

  atmos atmos [subcommand] <component> -s <stack> -- <native-flags>
+ 
+ Examples using aliases:
+   atmos tf plan <component> -s <stack>    # Same as: atmos terraform plan
+   atmos hf diff <component> -s <stack>    # Same as: atmos helmfile diff
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between ac22b72 and 1858b86.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (3)
  • go.mod (0 hunks)
  • tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (3 hunks)
  • tests/test-cases/log-level-validation.yaml (3 hunks)
💤 Files with no reviewable changes (1)
  • go.mod
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Build (macos-latest, macos)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Build (ubuntu-latest, linux)
  • GitHub Check: Analyze (go)
  • GitHub Check: Summary
🔇 Additional comments (5)
tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (4)

6-6: LGTM! Clear and consistent usage syntax.

The updated syntax atmos [sub-command] [flags] is more precise and aligns with the standardization goals. The explicit mention of [flags] helps users understand all possible command elements.


28-32: Excellent addition of command aliases!

The new SubCommand Aliases section introduces helpful shortcuts (hf for helmfile, tf for terraform) that will improve the CLI experience. These aliases target frequently used commands, making the CLI more efficient to use.


59-59: LGTM! Example follows the new terminology.

The example usage maintains consistency with the new [subcommand] terminology while preserving the clear structure for component and stack parameters.


62-62: LGTM! Help reference matches new terminology.

The help command reference consistently uses [subcommand], maintaining the standardized terminology throughout the documentation.

tests/test-cases/log-level-validation.yaml (1)

48-48: ⚠️ Potential issue

Review test expectations for valid log levels.

Spartan, these changes appear to be introducing incorrect behavior. The test cases for valid log levels (in config file, environment variable, and command line flag) now expect failure (exit code 1) where they previously expected success (exit code 0). Valid inputs should succeed, not fail.

Consider the following points:

  • Valid log levels like "Debug" and "Info" are legitimate values
  • Failing on valid input would break existing functionality
  • This change contradicts standard CLI behavior where valid inputs succeed

Let's verify the intended behavior:

Also applies to: 64-64, 80-80

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

LGTM

@aknysh aknysh merged commit b8f480f into main Jan 18, 2025
44 checks passed
@aknysh aknysh deleted the feature/dev-2896-incorrect-output-for-atmos-validate branch January 18, 2025 18:54
Copy link

These changes were released in v1.152.0.

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