refactor checkAtmosConfig #861
Conversation
osterman
requested review from
aknysh
and removed request for
Gowiem and
jamengual
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of command configuration checks across multiple Atmos CLI commands. The primary modification involves removing the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
By Sparta's shield, this refactoring looks clean and methodical! The changes maintain the spirit of the original implementation while introducing a more flexible configuration retrieval mechanism. Onward! 🛡️ 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 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
|
mcalhoun
added
the
no-release
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/cmd_utils.go(3 hunks)cmd/helmfile.go(1 hunks)cmd/root.go(5 hunks)cmd/terraform.go(1 hunks)cmd/version.go(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/terraform.go
🧰 Additional context used
📓 Learnings (2)
cmd/version.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/version.go:34-44
Timestamp: 2024-12-13T15:28:13.630Z
Learning: In `cmd/version.go`, when handling the `--check` flag in the `versionCmd`, avoid using `CheckForAtmosUpdateAndPrintMessage(cliConfig)` as it updates the cache timestamp, which may not be desired in this context.
cmd/cmd_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🔇 Additional comments (6)
cmd/root.go (3)
22-23: Strong type safety for context keys!
Good practice using a custom type for context keys to prevent accidental collisions.
127-131: Excellent refactoring of configuration handling!
The changes effectively centralize config validation and make it available throughout the command tree. This is a significant improvement that:
- Reduces redundant config checks
- Ensures consistent config state
- Makes the config easily accessible to all subcommands
95-98: Verify error handling for config initialization
The error handling looks correct, but we should ensure the true parameter passed to InitCliConfig is documented.
Consider adding a named constant for the boolean parameter to improve code readability:
-atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, true)
+const loadCustomCommands = true
+atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, loadCustomCommands)✅ Verification successful
Boolean parameter is well-documented, but could be more explicit
The processStacks parameter is documented in the function's context, but let's make it more explicit for better maintainability:
-atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, true)
+// Set to true to process custom commands from config
+const processCustomCommands = true
+atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, processCustomCommands)The function's documentation in pkg/config/config.go clearly shows its purpose of finding and merging CLI configurations from various sources (system dir, home dir, current dir, ENV vars). The boolean parameter is consistently used across the codebase, with false only used in describe_config.go where stack processing isn't needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the parameter is documented in the function definition
ast-grep --pattern 'func InitCliConfig($_, $bool) {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Let's search for the InitCliConfig function definition with ripgrep for better context
rg "func InitCliConfig" -A 5
# Also search for any documentation comments above the function
rg "InitCliConfig" -B 3
Length of output: 18711
go.mod (1)
89-89: Verify AWS SSM SDK dependency usage.
The addition of the AWS SSM SDK suggests new AWS Systems Manager functionality. Let's verify its usage in the codebase.
cmd/cmd_utils.go (2)
Line range hint 367-383: LGTM! Good refactoring of configuration handling.
The refactoring improves the function by:
- Removing global state dependency
- Making configuration flow explicit through parameters
- Maintaining existing error handling pattern (as per previous feedback)
479-481: LGTM! Consistent with the configuration refactoring.
The changes align with the broader refactoring by retrieving the configuration from the command context instead of global state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/root.go (2)
87-90: Request for clarity on the boolean parameter, fellow warrior! 🤔While moving the configuration initialization to the root level is a solid improvement, the
trueparameter incfg.InitCliConfig(schema.ConfigAndStacksInfo{}, true)needs clarification. Consider adding a comment explaining its purpose or using a named constant to make the intent clear.
119-123: Excellent refactoring of configuration flow, warrior! 🎯Moving the config validation to the root level and sharing it via context is a strong architectural improvement. Consider adding error handling for the context setup to maintain robustness:
checkAtmosConfig(&atmosConfig) // Now that we have fully parsed the Atmos config, add it to the context so it can be used by subcommands ctx := context.WithValue(context.Background(), contextKey("atmos_config"), atmosConfig) +if ctx == nil { + u.LogErrorAndExit(atmosConfig, errors.New("failed to create context with atmos config")) +} RootCmd.SetContext(ctx)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/cmd_utils.go(3 hunks)cmd/helmfile.go(2 hunks)cmd/root.go(4 hunks)cmd/terraform.go(2 hunks)cmd/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/helmfile.go
- cmd/terraform.go
- cmd/cmd_utils.go
- cmd/version.go
🔇 Additional comments (2)
cmd/root.go (2)
22-23: Strong work on type safety, warrior! 💪
The contextKey type alias is a solid defensive programming practice that prevents accidental context key collisions. This follows Go's best practices for context value keys.
107-116: Verify error handling consistency across command processing! 🛡️
The error handling for custom commands and aliases looks robust. Let's verify it's consistent across the codebase.
✅ Verification successful
Error handling is consistent and well-implemented! 💪
The error handling in both processCustomCommands and processCommandAliases follows a robust and consistent pattern:
processCustomCommandsproperly propagates errors up the call chainprocessCommandAliasescorrectly usesLogErrorAndExitfor critical execution failures- Both functions maintain consistent error handling with the rest of the codebase in
cmd/root.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other error handling patterns in command processing
rg "LogErrorAndExit.*atmosConfig" --type go
Length of output: 45
Script:
#!/bin/bash
# Let's search for error handling patterns more broadly
rg "LogErrorAndExit" -A 2 --type go
# Also check for the processCustomCommands and processCommandAliases usage
ast-grep --pattern 'processCustomCommands($$$)'
ast-grep --pattern 'processCommandAliases($$$)'
# Look for similar error handling patterns
rg "if err != nil \{" -A 2 -B 2 --type go
Length of output: 80007
Script:
#!/bin/bash
# Let's check the error handling in processCustomCommands and processCommandAliases
ast-grep --pattern 'func processCustomCommands($$$)'
ast-grep --pattern 'func processCommandAliases($$$)'
# Also check for similar error handling patterns in cmd/root.go
rg "LogErrorAndExit" -B 2 -A 2 cmd/root.go
Length of output: 7190
|
@mcalhoun please resolve the conflicts |
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
|
Closing this PR in favor of #869 |
what
Refactor the
checkAtmosConfigcode so that the config is loaded once in the root command, checked and added to the context for other commands to use.why
This code was being called multiple times, which decreased performance in was unnecessary.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor