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

Merge atmos specific and terraform help documentation #857

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

Conversation

samtholiya
Copy link
Collaborator

@samtholiya samtholiya commented Dec 14, 2024

what

Current:
image

After this change :
image


image

  • Merge help for terraform and atmos
  • Updated tab mechanism
  • Fade out not supported commands

why

  • We want to help customers show both native terraform commands and atmos terraform commands

references

Summary by CodeRabbit

  • New Features

    • Enhanced help command output with a styled logo for the "atmos" command.
    • Improved error handling and user feedback for the terraform command.
    • Introduced a comprehensive set of Terraform commands with detailed descriptions and examples.
    • Dynamic generation of help documentation based on command properties.
    • Enhanced command formatting capabilities to indicate unsupported commands.
  • Bug Fixes

    • Simplified help output for "terraform" commands to reduce clutter.
    • Removed automatic update checks from help command execution.
  • Documentation

    • Updated command descriptions for clarity and detail.
  • Refactor

    • Improved command formatting and usage templates for better representation of command availability.
  • Chores

    • Added TODO comments for future improvements in help processing.

@osterman
Copy link
Member

I wonder why your terminal is not in color

@osterman
Copy link
Member

osterman commented Dec 15, 2024

Oh, I see now I didn't realize we were not using cobra's built-in mechanism.

Can you check why we are not using that because that means we have a very different implementation for this one off command!

I like what you're doing more than what we had. However, it looks nothing like atmos --help

@osterman
Copy link
Member

Is the current wrapping okay? (i don't think so but because the ticket mentions we want a simple solution)

We have solved the wrapping issues in Cobra, so if we can get the menu back in there, that would be ideal.

@mergify mergify bot added the triage Needs triage label Dec 17, 2024
@samtholiya samtholiya force-pushed the feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help branch from 0fe021a to 4379eda Compare December 17, 2024 20:38
@osterman osterman removed the triage Needs triage label Dec 18, 2024
@mergify mergify bot added the triage Needs triage label Dec 18, 2024
{
Use: "apply",
Short: "Apply changes to infrastructure",
Long: "Apply the changes required to reach the desired state of the configuration. This will prompt for confirmation before making changes.",
Copy link
Member

Choose a reason for hiding this comment

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

I want to add an Example section here showing how to use apply, since it's one of the most common use-cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Long: "Create, list, select, or delete Terraform workspaces, which allow for separate states within the same configuration.",
},
{
Use: "clean",
Copy link
Member

Choose a reason for hiding this comment

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

This is a custom extension in atmos, so I want to keep our custom extensions in a separate section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you only care about user help output should be in seperate section. It is done. But if you meant even in code. I would like to ask why?

Long: "Displays the current version of Terraform installed on the system.",
},
{
Use: "varfile",
Copy link
Member

Choose a reason for hiding this comment

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

This should be in a new section for Atmos specific commands

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as for clean

u.PrintMessage(" - 'atmos terraform generate backends' command generates backend config files for all 'atmos' components in all stacks")
u.PrintMessage(" - 'atmos terraform generate varfile' command generates a varfile for an 'atmos' component in a stack")
u.PrintMessage(" - 'atmos terraform generate varfiles' command generates varfiles for all 'atmos' components in all stacks")
u.PrintMessage(" - 'atmos terraform shell' command configures an environment for an 'atmos' component in a stack and starts a new shell " +
Copy link
Member

Choose a reason for hiding this comment

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

We haven't documented shell subcommand. The command itself is documented, but not in the atmos terraform --help menu.

"allowing executing all native terraform commands inside the shell without using atmos-specific arguments and flags")
u.PrintMessage(" - double-dash '--' can be used to signify the end of the options for Atmos and the start of the additional " +
"native arguments and flags for the 'terraform' commands. " +
"For example: atmos terraform plan <component> -s <stack> -- -refresh=false -lock=false")
Copy link
Member

Choose a reason for hiding this comment

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

We should document the -- behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Knowing the section where we should add this info given that it is atmos wide concept would actually be beneficial.

Maybe we could do this at the footer of the help.
But let me know if you have any different thoughts

Copy link
Member

Choose a reason for hiding this comment

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

@osterman osterman removed the triage Needs triage label Dec 18, 2024
@mergify mergify bot added the triage Needs triage label Dec 18, 2024
@osterman osterman added minor New features that do not break anything and removed triage Needs triage labels Dec 18, 2024
Copy link

mergify bot commented Dec 18, 2024

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

@mergify mergify bot added the conflict This PR has conflicts label Dec 18, 2024
@samtholiya samtholiya force-pushed the feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help branch 2 times, most recently from b4b103a to 26d25fd Compare December 19, 2024 09:12
Copy link
Contributor

coderabbitai bot commented Dec 24, 2024

📝 Walkthrough

Walkthrough

The pull request introduces enhancements to the Atmos CLI's Terraform command handling and help system. It focuses on improving command execution, error handling, and help output for Terraform-related commands. Key modifications include restructuring the Terraform command framework, adding dynamic help template generation, and refining the help display logic across multiple files in the project.

Changes

File Change Summary
cmd/root.go Enhanced help function with conditional logo printing and original usage function preservation
cmd/terraform.go Replaced Run with RunE, improved error handling and command execution
cmd/terraform_commands.go Added comprehensive set of Terraform commands with detailed configuration
cmd/terraform_generate.go Updated command description and help text
internal/exec/help.go Removed detailed Terraform help messages
internal/exec/utils.go Modified help processing conditional logic
internal/tui/templates/base_template.go Introduced new template generation system for help output
internal/tui/templates/templater.go Enhanced command formatting and usage templates
cmd/cmd_utils.go Removed function for automatic update checks during help message display

Assessment against linked issues

Objective Addressed Explanation
Show Terraform help for supported commands
Differentiate supported and unsupported commands
Provide Cobra-style help for Atmos Terraform commands

Possibly related PRs

Suggested reviewers

  • osterman
  • aknysh
  • gberenice

📜 Recent review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 18a6d96 and 40b4dd3.

📒 Files selected for processing (1)
  • internal/tui/templates/base_template.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/tui/templates/base_template.go

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary or @auto-summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @auto-title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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 (3)
cmd/terraform.go (1)

26-92: Consider handling the error from cmd.Help()
Static analysis flags the unhandled return value from cmd.Help(). Even if it rarely fails, explicitly checking the error clarifies intent.

- cmd.Help()
+ if err := cmd.Help(); err != nil {
+   return fmt.Errorf("help error: %w", err)
+ }
🧰 Tools
🪛 golangci-lint (1.62.2)

81-81: Error return value of cmd.Help is not checked

(errcheck)

internal/tui/templates/templater.go (1)

50-104: Sorting logic for IsNotSupported
The sorting criteria places unsupported commands first. This is helpful. One minor suggestion: Ensure that the final order is intuitive for users, and consider grouping all unsupported commands at the bottom if that improves clarity.

cmd/terraform_commands.go (1)

1-288: Extensive Terraform command definitions
Defining all Terraform commands in one place is convenient. For long-term maintenance, you could consider auto-generating or grouping commands logically to reduce file size.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between b406027 and 48fee23.

📒 Files selected for processing (10)
  • cmd/root.go (1 hunks)
  • cmd/terraform.go (2 hunks)
  • cmd/terraform_commands.go (1 hunks)
  • cmd/terraform_generate.go (1 hunks)
  • examples/demo-localstack/atmos.yaml (1 hunks)
  • internal/exec/help.go (0 hunks)
  • internal/exec/terraform.go (1 hunks)
  • internal/exec/utils.go (1 hunks)
  • internal/tui/templates/base_template.go (1 hunks)
  • internal/tui/templates/templater.go (3 hunks)
💤 Files with no reviewable changes (1)
  • internal/exec/help.go
🧰 Additional context used
📓 Learnings (1)
internal/exec/utils.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
🪛 golangci-lint (1.62.2)
cmd/root.go

155-155: Error return value is not checked

(errcheck)

cmd/terraform.go

81-81: Error return value of cmd.Help is not checked

(errcheck)

🔇 Additional comments (18)
cmd/terraform.go (6)

4-5: Imports look fine
These imports provide necessary functionality for printing and do not introduce any issues.


10-10: New import for templates
This import is appropriately placed for usage template generation.


13-13: Coloredcobra import
Using an external library for colorizing output is beneficial.


23-23: Switch to RunE
Using RunE is a solid choice for improved error handling.


24-24: Blank line
No action needed here.


99-99: attachTerraformCommands call
This method call seamlessly integrates the Terraform commands.

internal/tui/templates/templater.go (4)

6-6: Import of "sort"
Adding sort functionality is appropriate for the new command sorting logic.


29-33: New styles for unsupported commands
Defining separate styles ensures clear UI distinction for unsupported commands.


37-46: Additional parameter in formatCommand
Adding IsNotSupported effectively customizes command rendering.


117-126: SetCustomUsageFunc updates
Switching to a newly generated usage template centralizes formatting and fosters consistency.

cmd/terraform_generate.go (1)

9-19: Enhanced usage descriptions
Providing a detailed explanation of subcommands clarifies usage.

internal/tui/templates/base_template.go (1)

1-80: New base template
This file introduces a straightforward solution for assembling custom help templates. The approach is maintainable, and the clear breakdown into sections aids in customizing the help output.

internal/exec/terraform.go (1)

20-20: Good consistent CLI flag update.

Switching from a single dash to a double dash aligns with standard CLI conventions. This should reduce confusion and improve compatibility.

internal/exec/utils.go (1)

223-223: Ensure help logic handles Terraform requests as intended.

Excluding the “terraform” component from the standard help flow seems intentional. Please confirm all valid Terraform help queries remain supported.

examples/demo-localstack/atmos.yaml (3)

37-38: Consistent usage of double-dash auto approve flag.

Switching to “--auto-approve” aligns with the recent updates in the codebase.


41-42: Consistent usage of double-dash auto approve flag.

Switching to “--auto-approve” aligns with the recent updates in the codebase.


45-46: Consistent usage of double-dash auto approve flag.

Switching to “--auto-approve” aligns with the recent updates in the codebase.

cmd/root.go (1)

141-141: Preserving original usage function is a clean approach.

Saving oldUsageFunc helps maintain fallback usage behavior without redundancy.

cmd/root.go Outdated Show resolved Hide resolved
@samtholiya samtholiya force-pushed the feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help branch from 7608538 to 4bf4346 Compare December 26, 2024 22:00
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 (6)
cmd/terraform_commands.go (6)

15-17: Include examples of common flags in the help text

This would help newcomers see more usage patterns and clarify common options for the plan command.


32-41: Add a usage example for clarity

A quick example, such as "atmos terraform workspace -s ", would be helpful for illustrating proper usage.


57-60: Consider confirming before auto-approve

Running apply with auto-approve can lead to unintended resource changes. Adding a safety prompt or user confirmation step might help avoid mistakes.


62-65: Enhance help info with an example

You might add an example showing how to open a shell for a specific component, e.g. "atmos terraform shell -s ", to help new users.


257-265: Adjust parent command call to clarify execution flow

Right now, the child's RunE calls the parent's RunE using potentially modified arguments. Consider directly calling cmd_.RunE or providing a fallback Usage if the parent’s RunE is nil. This keeps the command’s logic more intuitive.

 cmd.RunE = func(cmd_ *cobra.Command, args []string) error {
   if len(os.Args) > 3 {
     args = os.Args[2:]
   }
-  if parentCmd.RunE != nil {
-    return parentCmd.RunE(parentCmd, args)
+  if cmd_.RunE != nil {
+    return cmd_.RunE(cmd_, args)
   }
   return nil
 }

270-285: Provide usage examples for newly added flags

Including usage examples for each flag under deploy, apply, and clean will help users learn how to invoke them in real scenarios.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7608538 and 4bf4346.

📒 Files selected for processing (2)
  • cmd/root.go (1 hunks)
  • cmd/terraform_commands.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/root.go

…terraform-help' of https://github.com/cloudposse/atmos into feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
@samtholiya samtholiya force-pushed the feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help branch from 4bf4346 to 7765e3f Compare December 26, 2024 22:45
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)
cmd/terraform.go (5)

28-28: Remove redundant commented code

This commented line is redundant since checkAtmosConfig() is already called at line 86.

-	//checkAtmosConfig()

30-37: Consider simplifying double dash argument handling

The current implementation works well but could be more concise using array slicing with a single conditional.

-	var argsAfterDoubleDash []string
-	var finalArgs = args
-
-	doubleDashIndex := lo.IndexOf(args, "--")
-	if doubleDashIndex > 0 {
-		finalArgs = lo.Slice(args, 0, doubleDashIndex)
-		argsAfterDoubleDash = lo.Slice(args, doubleDashIndex+1, len(args))
-	}
+	doubleDashIndex := lo.IndexOf(args, "--")
+	finalArgs := args
+	argsAfterDoubleDash := []string{}
+	if doubleDashIndex > 0 {
+		finalArgs, argsAfterDoubleDash = args[:doubleDashIndex], args[doubleDashIndex+1:]
+	}

53-53: Fix typo in error message

There's a typo in the error message: "Unknkown" should be "Unknown".

-				fmt.Printf(`Error: Unknkown command %q for %q`+"\n", args[0], cmd.CommandPath())
+				fmt.Printf(`Error: Unknown command %q for %q`+"\n", args[0], cmd.CommandPath())

81-83: Handle potential error from cmd.Help()

While Help() errors are rare, we should handle them for completeness.

-		cmd.Help()
+		if err := cmd.Help(); err != nil {
+			return fmt.Errorf("failed to show help: %w", err)
+		}
🧰 Tools
🪛 golangci-lint (1.62.2)

81-81: Error return value of cmd.Help is not checked

(errcheck)


26-92: Excellent implementation aligning with PR objectives!

This implementation successfully:

  • Merges Atmos-specific and Terraform help documentation
  • Adds proper terminal colors through coloredcobra
  • Improves error handling and command structure

The changes align perfectly with the PR objectives and address the feedback from the PR comments about terminal colors.

Consider adding unit tests for the help text generation and command handling to ensure the behavior remains consistent as the codebase evolves.

🧰 Tools
🪛 golangci-lint (1.62.2)

81-81: Error return value of cmd.Help 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 4bf4346 and 7765e3f.

📒 Files selected for processing (3)
  • cmd/root.go (1 hunks)
  • cmd/terraform.go (2 hunks)
  • cmd/terraform_commands.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • cmd/root.go
  • cmd/terraform_commands.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/terraform.go

81-81: Error return value of cmd.Help is not checked

(errcheck)

🔇 Additional comments (2)
cmd/terraform.go (2)

23-23: Excellent move to RunE!

The switch from Run to RunE is a solid improvement, warrior! This allows proper error propagation up the command chain, making the CLI more robust.


59-79: Well-structured help template with excellent styling!

The help template generation with coloredcobra integration looks great! This aligns perfectly with the PR objective of merging Atmos-specific and Terraform help documentation. The color scheme choices will make the help output more readable and user-friendly.

Copy link

mergify bot commented Dec 27, 2024

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

@mergify mergify bot added the conflict This PR has conflicts label Dec 27, 2024
@mergify mergify bot removed the conflict This PR has conflicts label Dec 27, 2024
@aknysh
Copy link
Member

aknysh commented Dec 27, 2024

@samtholiya thanks for the PR, I'll review it.

Please make sure the new help includes all the terraform providers xxxx command added yesterday in this release
https://github.com/cloudposse/atmos/releases/tag/v1.132.0

(help.go was modified, but after this PR is reviewed and merged, nobody should add anything to help.go)

thank you

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

🧹 Nitpick comments (2)
internal/tui/templates/base_template.go (2)

21-27: Consider adding error checks when building templates.

While minimal, there's a risk that unexpected sections or invalid data could disrupt template building. For instance, poorly formatted strings might cause runtime issues. You could log or safely handle such anomalies.


29-88: Potential for repeated string logic.

getSection switches on the same enumeration in multiple places, each returning fairly similar strings. If these usage sections expand, you might consider extracting repeated blocks into helper functions or otherwise modularizing. This can streamline changes and testing, while still keeping logic local.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d0351a8 and 1d8fe0e.

📒 Files selected for processing (7)
  • cmd/root.go (1 hunks)
  • cmd/terraform.go (2 hunks)
  • cmd/terraform_commands.go (1 hunks)
  • cmd/terraform_generate.go (1 hunks)
  • internal/exec/help.go (0 hunks)
  • internal/exec/utils.go (3 hunks)
  • internal/tui/templates/base_template.go (1 hunks)
💤 Files with no reviewable changes (1)
  • internal/exec/help.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/exec/utils.go
  • cmd/terraform_commands.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/terraform.go

90-90: Error return value of actualCmd.Help is not checked

(errcheck)

cmd/root.go

157-157: Error return value of command.Usage is not checked

(errcheck)

🔇 Additional comments (6)
internal/tui/templates/base_template.go (1)

5-19: Well-organized enumeration of constants.

It’s a neat enumeration of help template sections that fosters clarity. Having distinct cases for each section keeps the code maintainable, straightforward, and easy to extend in the future.

cmd/terraform_generate.go (2)

9-19: Clearer command descriptions!

Expanding the Short and Long fields clarifies command intentions, thus improving usability. Thanks for making these improvements.


21-23: Unified command execution approach.

Using RunE for error returns is consistent with broader Cobra best practices, allowing streamlined error handling. This fosters more robust code.

cmd/terraform.go (2)

23-26: Excellent transition to RunE for error propagation.

Leveraging RunE ensures that returned errors can be handled uniformly, reducing hidden failures.


28-36: Potential concurrency consideration for Contains.

While Contains is a simple helper, if used in parallel contexts, consider synchronization or a concurrent-safe design, especially if future expansions occur. Currently, it seems safe as it just reads from a slice.

cmd/root.go (1)

141-159: Commendably structured custom help function.

Capturing the original usage function and selectively calling it for non-atmos commands is a solid approach, preventing recursion or confusion.

🧰 Tools
🪛 golangci-lint (1.62.2)

157-157: Error return value of command.Usage is not checked

(errcheck)

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

♻️ Duplicate comments (2)
cmd/terraform.go (1)

92-92: ⚠️ Potential issue

Handle Help() error return value.

The Help() method returns an error that should be handled.

-		actualCmd.Help()
+		if err := actualCmd.Help(); err != nil {
+			u.LogErrorAndExit(schema.AtmosConfiguration{}, err)
+		}
🧰 Tools
🪛 golangci-lint (1.62.2)

92-92: Error return value of actualCmd.Help is not checked

(errcheck)

cmd/root.go (1)

136-147: ⚠️ Potential issue

Handle Usage() error return value.

The Usage() method returns an error that should be handled.

-			command.Usage()
+			if err := command.Usage(); err != nil {
+				u.LogErrorAndExit(schema.AtmosConfiguration{}, err)
+			}
🧰 Tools
🪛 golangci-lint (1.62.2)

146-146: Error return value of command.Usage is not checked

(errcheck)

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

28-36: Consider using lo.Contains for consistency.

Since the codebase already imports the lo package (used in line 42), consider using lo.Contains instead of implementing a custom Contains function.

-func Contains(slice []string, target string) bool {
-	for _, item := range slice {
-		if item == target {
-			return true
-		}
-	}
-	return false
-}

48-64: Fix typo in error message.

There's a typo in the error message: "Unknkown" should be "Unknown".

-					fmt.Printf(`Error: Unknkown command %q for %q`+"\n", args[0], cmd.CommandPath())
+					fmt.Printf(`Error: Unknown command %q for %q`+"\n", args[0], cmd.CommandPath())
internal/tui/templates/templater.go (1)

87-102: Simplify sorting logic for better readability.

The current sorting implementation can be simplified using lo.XOR for better readability.

-		if iHasKey && !jHasKey {
-			return true
-		}
-		if !iHasKey && jHasKey {
-			return false
-		}
-		// If both or neither have the key, maintain original order
-		return i < j
+		if lo.XOR(iHasKey, jHasKey) {
+			return iHasKey
+		}
+		return i < j
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1d8fe0e and a8d6345.

📒 Files selected for processing (5)
  • cmd/cmd_utils.go (0 hunks)
  • cmd/root.go (1 hunks)
  • cmd/terraform.go (2 hunks)
  • internal/tui/templates/base_template.go (1 hunks)
  • internal/tui/templates/templater.go (3 hunks)
💤 Files with no reviewable changes (1)
  • cmd/cmd_utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/tui/templates/base_template.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/terraform.go

92-92: Error return value of actualCmd.Help is not checked

(errcheck)

cmd/root.go

146-146: Error return value of command.Usage is not checked

(errcheck)

🔇 Additional comments (2)
cmd/terraform.go (1)

23-25: LGTM! Proper error handling with RunE.

The transition from Run to RunE improves error handling by properly propagating errors up the command chain.

internal/tui/templates/templater.go (1)

29-33: LGTM! Clear visual distinction for unsupported commands.

The new styles effectively differentiate unsupported commands using a faded color (8), making it visually clear which commands are not supported.

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 (3)
cmd/terraform.go (3)

28-36: Consider using lo.Contains instead of custom implementation

Since the project already uses the lo library which provides this exact functionality, we could simplify this:

-// Contains checks if a slice of strings contains an exact match for the target string.
-func Contains(slice []string, target string) bool {
-	for _, item := range slice {
-		if item == target {
-			return true
-		}
-	}
-	return false
-}

And replace all usages with lo.Contains(slice, target).


60-60: Fix typo in error message

There's a typo in the error message: "Unknkown" should be "Unknown".

-fmt.Printf(`Error: Unknkown command %q for %q`+"\n", args[0], cmd.CommandPath())
+fmt.Printf(`Error: Unknown command %q for %q`+"\n", args[0], cmd.CommandPath())

66-89: Consider refactoring help template initialization

The help template initialization logic could be moved to a separate function for better maintainability. Also, the TODO comment should be addressed or removed if it's no longer relevant.

Consider extracting the template and color configuration into a separate function:

func initializeHelpTemplate(actualCmd *cobra.Command) {
    template := templates.GenerateFromBaseTemplate(actualCmd.Use, []templates.HelpTemplateSections{
        templates.LongDescription,
        // ... other sections ...
    })
    actualCmd.SetUsageTemplate(template)
    
    cc.Init(&cc.Config{
        RootCmd:  actualCmd,
        // ... color config ...
    })
}
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between a8d6345 and ed8b9f8.

📒 Files selected for processing (3)
  • cmd/helmfile.go (1 hunks)
  • cmd/terraform.go (2 hunks)
  • internal/exec/utils.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/exec/utils.go
🧰 Additional context used
📓 Learnings (1)
cmd/helmfile.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
🪛 golangci-lint (1.62.2)
cmd/helmfile.go

37-37: Error return value of cmd.Help is not checked

(errcheck)

cmd/terraform.go

92-92: Error return value of actualCmd.Help is not checked

(errcheck)

🔇 Additional comments (4)
cmd/helmfile.go (1)

35-38: LGTM on help condition enhancement!

The expanded condition info.SubCommand == "" && info.SubCommand2 == "" is a solid improvement, showing help when no subcommands are specified. This aligns well with the PR's goal of enhancing help documentation display.

🧰 Tools
🪛 golangci-lint (1.62.2)

37-37: Error return value of cmd.Help is not checked

(errcheck)

cmd/terraform.go (3)

23-25: Excellent change to RunE for proper error handling!

The switch from Run to RunE is a solid improvement that allows for proper error propagation in the command chain.


104-111: Solid command initialization!

The initialization follows Cobra best practices with proper flag setup and command attachment.


92-92: ⚠️ Potential issue

Handle Help() error return value

The Help() function returns an error that should be handled:

-actualCmd.Help()
+if err := actualCmd.Help(); err != nil {
+    return fmt.Errorf("failed to show help: %w", err)
+}

Likely invalid or redundant comment.

🧰 Tools
🪛 golangci-lint (1.62.2)

92-92: Error return value of actualCmd.Help is not checked

(errcheck)

cmd/helmfile.go Outdated 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

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

35-39: LGTM! Error handling looks solid, warrior! 💪

The expanded help condition and proper error handling for cmd.Help() are excellent improvements. This change aligns well with the PR's objective of enhancing help documentation.

However, consider adding a debug log before showing help to assist with troubleshooting:

 if info.NeedHelp || (info.SubCommand == "" && info.SubCommand2 == "") {
+    u.LogDebug("Displaying helmfile help due to empty subcommands or help flag")
     if err := cmd.Help(); err != nil {
         u.LogErrorAndExit(schema.AtmosConfiguration{}, err)
     }
     return
 }
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between ed8b9f8 and 18a6d96.

📒 Files selected for processing (2)
  • cmd/helmfile.go (1 hunks)
  • cmd/root.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/root.go
🧰 Additional context used
📓 Learnings (1)
cmd/helmfile.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.

@osterman
Copy link
Member

osterman commented Jan 2, 2025

The argument passed is --asdasd is not a command. It's a flag. We should use precise language.

❯ ../../build/atmos terraform --asdasd
Error: Unknkown command "--asdasd" for "atmos terraform"
Run 'atmos terraform --help' for usage
unknown command "--asdasd" for "atmos terraform"

@osterman
Copy link
Member

osterman commented Jan 2, 2025

I tried to use the double-dash with -help and it didn't work.

 ../../build/atmos terraform plan -- -help
image

@osterman
Copy link
Member

osterman commented Jan 2, 2025

Running atmos terraform plan without any arguments emits help, rather than usage.

image

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