Use Viper's built-in configuration handling #1074
Conversation
📝 WalkthroughWalkthroughThis pull request streamlines the Atmos CLI configuration process. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 3
🧹 Nitpick comments (2)
pkg/config/load.go (2)
24-105: Consider DRY improvements in LoadConfig.
LoadConfigis comprehensive. Repeated patterns for reading config from various sources could be consolidated to simplify and reduce potential errors.
178-196: Double-check function naming.
readEnvAmosConfigPathmight be a small naming slip compared to "ATMOS_CLI_CONFIG_PATH". For consistency, consider aligning the naming.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/config.go(1 hunks)pkg/config/load.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/load.go
20-20: field paths is unused
(unused)
21-21: field atmosFileNames is unused
(unused)
211-211: func applyDefaultConfiguration is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/config/load.go (4)
107-115: Defaults look good.
The default configuration assignments are clear and logically grouped. They ensure sensible fallback values for logs and Terraform usage.
117-139: System config logic is straightforward.
Merging from system directories is handled cleanly. This code helps ensure that global defaults are recognized.
141-158: Home config approach is clear.
The code thoroughly checks for the config file in the user’s home directory and logs aConfigFileNotFoundErrorgracefully.
160-176: WorkDir config merges.
Similar pattern asreadSystemConfigandreadHomeConfig. Make sure the repeated checks remain consistent if the logic evolves.pkg/config/config.go (1)
118-118: Refactored loading is clear.
CallingLoadConfigsimplifiesInitCliConfigsignificantly. The error handling is correct, and the flow remains consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/config/default.go (1)
108-125: Function Marked as UnusedThe static analysis tool flags
applyDefaultConfigurationas unused. If this is intentional, consider removing it. Else, ensure it’s invoked where appropriate to apply defaults.🧰 Tools
🪛 golangci-lint (1.62.2)
109-109: func
applyDefaultConfigurationis unused(unused)
pkg/config/load.go (2)
24-105: Consider enhancing debug logging.The LoadConfig function handles configuration well, but could benefit from additional debug logging to help troubleshoot configuration issues in production.
Add debug logging before attempting to load each configuration source:
func LoadConfig(configAndStacksInfo schema.ConfigAndStacksInfo) (schema.AtmosConfiguration, error) { v := viper.New() var atmosConfig schema.AtmosConfiguration + u.LogDebug("Initializing configuration loading") v.SetConfigType("yaml")
108-115: Consider adding more comprehensive defaults.The function sets essential defaults but could include additional safety defaults for timeouts and retries.
Consider adding these defaults:
func setDefaultConfiguration(v *viper.Viper) { v.SetDefault("components.helmfile.use_eks", true) v.SetDefault("components.terraform.append_user_agent", fmt.Sprintf("Atmos/%s (Cloud Posse; +https://atmos.tools)", version.Version)) v.SetDefault("settings.inject_github_token", true) v.SetDefault("logs.file", "/dev/stderr") v.SetDefault("logs.level", "Info") + v.SetDefault("components.terraform.operation_timeout", "30m") + v.SetDefault("components.terraform.retry_attempts", 3) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/config/config.go(1 hunks)pkg/config/default.go(1 hunks)pkg/config/load.go(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/test-cases/env.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
Learnt from: samtholiya
PR: cloudposse/atmos#955
File: tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden:0-0
Timestamp: 2025-01-19T15:49:15.593Z
Learning: In future commits, the help text for Atmos CLI commands should be limited to only show component and stack parameters for commands that actually use them. This applies to the example usage section in command help text.
🪛 golangci-lint (1.62.2)
pkg/config/default.go
109-109: func applyDefaultConfiguration is unused
(unused)
pkg/config/load.go
20-20: field paths is unused
(unused)
21-21: field atmosFileNames is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/config/default.go (3)
1-16: Imports and Packaging Look GoodNo issues with the package statement or imports, and the dependencies are relevant and used correctly.
18-22: Well-Defined Error VariableDefining
NotFoundprovides a clear message. Good user guidance on where to find sample configs.
23-105: Default Configuration Initialization Appears ValidThe default configuration covers all major sections comprehensively (Stacks, Components, Logs, etc.). No immediate concerns with naming conventions, structure, or duplication. Error handling seems sufficient.
tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden (1)
46-46: Path Update for YAML ConfigReferencing a specific file instead of a directory provides clarity. Confirm that references in code match this new path.
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
148-148: Consistent YAML Configuration ReferenceShifting to “atmos.yaml” reflects the updated configuration approach. No further concerns.
pkg/config/config.go (1)
27-28: Well done on the refactoring!The delegation of configuration loading to
LoadConfigimproves code organization while maintaining the same public interface. The post-processing steps are preserved, ensuring backward compatibility.pkg/config/load.go (2)
19-22: Remove unused fields from ConfigSources struct.The
pathsandatmosFileNamesfields are currently unused. Consider removing them or documenting their intended future use.🧰 Tools
🪛 golangci-lint (1.62.2)
20-20: field
pathsis unused(unused)
21-21: field
atmosFileNamesis unused(unused)
198-208: Fix error handling in mergeConfig function.The function currently discards errors from
MergeInConfig. This could mask configuration issues.tests/test-cases/env.yaml (1)
19-19: Good update to the test expectations!The updated path pattern correctly reflects the explicit YAML file requirement, aligning with the new configuration loading mechanism.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/config/default.go (1)
13-15: Enhance error message with environment variable name.The error message is helpful but could be more specific about which environment variable users can set.
- "\nYou can download a sample config and adapt it to your requirements from " + + "\nYou can set ATMOS_CLI_CONFIG_PATH environment variable or download a sample config from " + "https://raw.githubusercontent.com/cloudposse/atmos/main/examples/quick-start-advanced/atmos.yaml")pkg/config/load.go (4)
43-43: Remove redundant length check.The condition
if len(configFilePath) > 0is redundant sinceconfigFilePathis already assigned fromAtmosCliConfigPathwhich was checked for non-emptiness.- if len(configFilePath) > 0 { err := mergeConfig(v, configFilePath, CliConfigFileName) - }
116-119: Improve Windows path handling.The Windows configuration path handling could be more robust by:
- Validating that the directory exists
- Using filepath.Join for path construction
appDataDir := os.Getenv(WindowsAppDataEnvVar) if len(appDataDir) > 0 { + if _, err := os.Stat(appDataDir); err == nil { configFilePath = appDataDir + } }
126-132: Extract common error handling pattern.The same error handling pattern for
viper.ConfigFileNotFoundErroris repeated in multiple places. Consider extracting it into a helper function.+func handleConfigNotFoundError(err error) error { + if err != nil { + switch err.(type) { + case viper.ConfigFileNotFoundError: + return nil + default: + return err + } + } + return nil +}Also applies to: 145-151, 163-169, 179-186
47-47: Standardize logging patterns.The trace logging uses inconsistent string formatting patterns. Some use
fmt.Sprintfwhile others use string concatenation.Standardize on
fmt.Sprintf:- u.LogTrace(fmt.Sprintf("Not Found config file %s", configFilePath)) + u.LogTrace(fmt.Sprintf("Config file not found: %s", configFilePath)) - u.LogTrace(fmt.Sprintf("Not Found ENV var ATMOS_CLI_CONFIG_PATH=%s", configFilePath)) + u.LogTrace(fmt.Sprintf("Environment variable not found: ATMOS_CLI_CONFIG_PATH=%s", configFilePath)) - u.LogTrace(fmt.Sprintf("Found ENV var ATMOS_CLI_CONFIG_PATH=%s", configFilePath)) + u.LogTrace(fmt.Sprintf("Found environment variable: ATMOS_CLI_CONFIG_PATH=%s", configFilePath))Also applies to: 181-181, 187-187
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/default.go(1 hunks)pkg/config/load.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/config/default.go (1)
95-95: Specify units for version check timeout.The timeout value of 1000 lacks unit specification. Is this milliseconds, seconds, or something else?
Consider adding a comment or using a more explicit constant like
DefaultVersionCheckTimeoutMs.pkg/config/load.go (1)
197-203: Do not discard the error from MergeInConfig.All outcomes of
MergeInConfigreturnnil, ignoring potential errors. That can mask failures.
osterman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/config/load.go (2)
184-193:⚠️ Potential issueDo not discard the error from MergeInConfig.
All outcomes of
MergeInConfigreturnnil, ignoring potential errors. That can mask failures.Apply this fix:
func mergeConfig(v *viper.Viper, path string, fileName string) error { v.AddConfigPath(path) v.SetConfigName(fileName) err := v.MergeInConfig() if err != nil { - return nil + return err } return nil }
76-82:⚠️ Potential issueGuard against potential nil pointer dereference.
The code checks
filepath.IsAbs(atmosConfig.CliConfigPath)without first verifying thatCliConfigPathis not empty, which could lead to a panic.Apply this fix:
- if atmosConfig.CliConfigPath != "" && !filepath.IsAbs(atmosConfig.CliConfigPath) { + if atmosConfig.CliConfigPath != "" && !filepath.IsAbs(atmosConfig.CliConfigPath) { absPath, err := filepath.Abs(atmosConfig.CliConfigPath) if err != nil { return atmosConfig, err } atmosConfig.CliConfigPath = absPath }
🧹 Nitpick comments (1)
pkg/config/load.go (1)
24-92: Consider enhancing error messages for better debugging.The
LoadConfigfunction could provide more context in error messages to help users troubleshoot configuration issues. Consider wrapping errors with additional context about which configuration source failed to load.Example enhancement:
err := readSystemConfig(v) if err != nil { - return atmosConfig, err + return atmosConfig, fmt.Errorf("failed to load system config: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/config.go(1 hunks)pkg/config/load.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (22)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [mock-macos] examples/demo-component-versions
- GitHub Check: [mock-macos] examples/demo-atlantis
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: [mock-linux] examples/demo-context
- GitHub Check: [mock-linux] examples/demo-component-versions
- GitHub Check: [lint] demo-context
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Docker Lint
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/config/config.go (1)
18-18: Well done on simplifying configuration loading!The refactoring to use
LoadConfigeffectively leverages Viper's built-in configuration capabilities while maintaining the same functionality. This change aligns perfectly with the PR objectives.
| if err != nil { | ||
| switch err.(type) { | ||
| case viper.ConfigFileNotFoundError: | ||
| log.Debug("config not found ENV var ATMOS_CLI_CONFIG_PATH", "file", configFilePath) |
There was a problem hiding this comment.
| log.Debug("config not found ENV var ATMOS_CLI_CONFIG_PATH", "file", configFilePath) | |
| log.Debug("config not found from ENV”, “ATMOS_CLI_CONFIG_PATH", configFilePath) |
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
|
While we are splitting these functions. It would be great if we could followup with the following enhancement. |
what
-refactor the InitCliConfig function to use Viper's built-in configuration loading instead of manually processing each config file
why
Files (YAML,YML, JSON, TOML, etc.).
Environment variables.
Flags.
Remote sources
This makes it easier to extend the configuration system in the future.
references
Summary by CodeRabbit
New Features
Refactor