Merge atmos specific and terraform help documentation #857
Conversation
|
I wonder why your terminal is not in color |
|
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 |
…-also-show-terraform-help
We have solved the wrapping issues in Cobra, so if we can get the menu back in there, that would be ideal. |
samtholiya
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
0fe021a to
4379eda
Compare
cmd/terraform_native_commands.go
Outdated
| { | ||
| 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.", |
There was a problem hiding this comment.
I want to add an Example section here showing how to use apply, since it's one of the most common use-cases.
cmd/terraform_native_commands.go
Outdated
| { | ||
| Use: "plan", | ||
| Short: "Show changes required by the current configuration", | ||
| Long: "Generate an execution plan, which shows what actions Terraform will take to reach the desired state of the configuration.", |
There was a problem hiding this comment.
I want to add an Example section here showing how to use plan, since it's one of the most common use-cases.
cmd/terraform_native_commands.go
Outdated
| { | ||
| Use: "init", | ||
| Short: "Prepare your working directory for other commands", | ||
| Long: "Initialize the working directory containing Terraform configuration files. It will download necessary provider plugins and set up the backend.", |
There was a problem hiding this comment.
| Long: "Initialize the working directory containing Terraform configuration files. It will download necessary provider plugins and set up the backend.", | |
| Long: "Initialize the working directory containing Terraform configuration files. It will download necessary provider plugins and set up the backend. Note, that Atmos will automatically call init for you, when running `plan` and `apply` commands.", |
cmd/terraform_native_commands.go
Outdated
| { | ||
| Use: "workspace", | ||
| Short: "Manage Terraform workspaces", | ||
| Long: "Create, list, select, or delete Terraform workspaces, which allow for separate states within the same configuration.", |
There was a problem hiding this comment.
| Long: "Create, list, select, or delete Terraform workspaces, which allow for separate states within the same configuration.", | |
| Long: "Create, list, select, or delete Terraform workspaces, which allow for separate states within the same configuration. Note, Atmos will automatically select the workspace, if configured.", |
cmd/terraform_native_commands.go
Outdated
| Long: "Create, list, select, or delete Terraform workspaces, which allow for separate states within the same configuration.", | ||
| }, | ||
| { | ||
| Use: "clean", |
There was a problem hiding this comment.
This is a custom extension in atmos, so I want to keep our custom extensions in a separate section.
There was a problem hiding this comment.
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?
cmd/terraform_native_commands.go
Outdated
| Long: "Displays the current version of Terraform installed on the system.", | ||
| }, | ||
| { | ||
| Use: "varfile", |
There was a problem hiding this comment.
This should be in a new section for Atmos specific commands
There was a problem hiding this comment.
Same as for clean
cmd/terraform_native_commands.go
Outdated
| Long: "Load variable definitions from a specified file and use them in the configuration.", | ||
| }, | ||
| { | ||
| Use: "write varfile", |
There was a problem hiding this comment.
This should be in a new section for Atmos specific commands
There was a problem hiding this comment.
Same as for clean
| " --force Forcefully delete Terraform state files and directories without interaction\n" + | ||
| " --skip-lock-file Skip deleting the '.terraform.lock.hcl' file\n\n" + |
There was a problem hiding this comment.
We need to make sure these are documented
| u.PrintMessage("\nAdditions and differences from native terraform:") | ||
| u.PrintMessage(" - before executing other 'terraform' commands, 'atmos' runs 'terraform init'") | ||
| u.PrintMessage(" - you can skip over atmos calling 'terraform init' if you know your project is already in a good working state by using " + | ||
| "the '--skip-init' flag like so 'atmos terraform <command> <component> -s <stack> --skip-init") |
There was a problem hiding this comment.
we need to make sure --skip-init is documented
| u.PrintMessage(" - 'atmos terraform deploy' command executes 'terraform apply -auto-approve' (sets the '-auto-approve' flag when running 'terraform apply')") | ||
| u.PrintMessage(" - 'atmos terraform deploy' command supports '--deploy-run-init=true/false' flag to enable/disable running 'terraform init' " + | ||
| "before executing the command") | ||
| u.PrintMessage(" - 'atmos terraform apply' and 'atmos terraform deploy' commands support '--from-plan' flag. If the flag is specified, " + |
There was a problem hiding this comment.
We need to make sure --from-plan is documented
| u.PrintMessage(" - 'atmos terraform deploy' command executes 'terraform apply -auto-approve' (sets the '-auto-approve' flag when running 'terraform apply')") | ||
| u.PrintMessage(" - 'atmos terraform deploy' command supports '--deploy-run-init=true/false' flag to enable/disable running 'terraform init' " + | ||
| "before executing the command") |
There was a problem hiding this comment.
We haven't documented deploy command and it's flags
| 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 " + |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
We should document the -- behavior.
There was a problem hiding this comment.
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
osterman
added
minor
|
💥 This pull request now has conflicts. Could you fix it @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
dev-2821-atmos-terraform-help-should-also-show-terraform-help
samtholiya
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
fb916d8 to
d7cabbd
Compare
samtholiya
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
437e23a to
84a1033
Compare
samtholiya
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
84a1033 to
c719d86
Compare
…-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
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
457e5a9 to
48fee23
Compare
📝 WalkthroughWalkthroughThe 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
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/terraform.go (1)
26-92: Consider handling the error fromcmd.Help()
Static analysis flags the unhandled return value fromcmd.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.Helpis 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
📒 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
| if command.Use != "terraform" { | ||
| err := tuiUtils.PrintStyledText("ATMOS") | ||
| if err != nil { | ||
| u.LogErrorAndExit(schema.AtmosConfiguration{}, err) | ||
| } | ||
| } | ||
| // TODO: find a better way to do this if possible | ||
| if command.Use == "terraform" { | ||
| oldUsageFunc(command) | ||
| return | ||
| } else { | ||
| b.HelpFunc(command, strings) |
There was a problem hiding this comment.
Check the error return of old usage function.
According to lint feedback, the error return value from oldUsageFunc(command) is not verified. If it is meaningful, consider error handling to avoid silent failures.
- oldUsageFunc(command)
+ if err := oldUsageFunc(command); err != nil {
+ u.LogErrorAndExit(schema.AtmosConfiguration{}, err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if command.Use != "terraform" { | |
| err := tuiUtils.PrintStyledText("ATMOS") | |
| if err != nil { | |
| u.LogErrorAndExit(schema.AtmosConfiguration{}, err) | |
| } | |
| } | |
| // TODO: find a better way to do this if possible | |
| if command.Use == "terraform" { | |
| oldUsageFunc(command) | |
| return | |
| } else { | |
| b.HelpFunc(command, strings) | |
| if command.Use != "terraform" { | |
| err := tuiUtils.PrintStyledText("ATMOS") | |
| if err != nil { | |
| u.LogErrorAndExit(schema.AtmosConfiguration{}, err) | |
| } | |
| } | |
| // TODO: find a better way to do this if possible | |
| if command.Use == "terraform" { | |
| if err := oldUsageFunc(command); err != nil { | |
| u.LogErrorAndExit(schema.AtmosConfiguration{}, err) | |
| } | |
| return | |
| } else { | |
| b.HelpFunc(command, strings) |
🧰 Tools
🪛 golangci-lint (1.62.2)
155-155: Error return value is not checked
(errcheck)
examples/demo-localstack/atmos.yaml
Outdated
| - atmos terraform apply demo -s dev --auto-approve | ||
| - atmos terraform destroy demo -s dev --auto-approve | ||
| # Test staging stack | ||
| - atmos terraform plan demo -s staging | ||
| - atmos terraform apply demo -s staging -auto-approve | ||
| - atmos terraform destroy demo -s staging -auto-approve | ||
| - atmos terraform apply demo -s staging --auto-approve | ||
| - atmos terraform destroy demo -s staging --auto-approve | ||
| # Test prod stack | ||
| - atmos terraform plan demo -s prod | ||
| - atmos terraform apply demo -s prod -auto-approve | ||
| - atmos terraform destroy demo -s prod -auto-approve | ||
| - atmos terraform apply demo -s prod --auto-approve | ||
| - atmos terraform destroy demo -s prod --auto-approve | ||
|
|
There was a problem hiding this comment.
This seems wrong to me.
Terraform uses -auto-approve, not --auto-approve
There was a problem hiding this comment.
At a minimum, this will break existing implementations.
samtholiya
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
7608538 to
4bf4346
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
cmd/terraform_commands.go (6)
15-17: Include examples of common flags in the help textThis would help newcomers see more usage patterns and clarify common options for the plan command.
32-41: Add a usage example for clarityA quick example, such as "atmos terraform workspace -s ", would be helpful for illustrating proper usage.
57-60: Consider confirming before auto-approveRunning 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 exampleYou 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 flowRight now, the child's RunE calls the parent's RunE using potentially modified arguments. Consider directly calling
cmd_.RunEor providing a fallbackUsageif 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 flagsIncluding usage examples for each flag under
deploy,apply, andcleanwill help users learn how to invoke them in real scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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
force-pushed
the
feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
branch
from
4bf4346 to
7765e3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cmd/terraform.go (5)
28-28: Remove redundant commented codeThis commented line is redundant since
checkAtmosConfig()is already called at line 86.- //checkAtmosConfig()
30-37: Consider simplifying double dash argument handlingThe 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 messageThere'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.Helpis 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.Helpis not checked(errcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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.
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
…-also-show-terraform-help
|
@samtholiya thanks for the PR, I'll review it. Please make sure the new help includes all the ( thank you |
what
Current:
After this change :
not supportedcommandswhy
references
Summary by CodeRabbit
New Features
terraformcommand.Bug Fixes
Documentation
Refactor
Chores