Refine Atmos Configuration Handling to Support Imports, Remote Imports, and Configuration Directories #808
Conversation
haitham911
requested review from
johncblandii,
RoseSecurity,
aknysh and
osterman
📝 WalkthroughWalkthroughThe changes refactor Atmos’s configuration management by consolidating configuration loading into a centralized ConfigLoader. The new logic deep merges configurations from various sources—including local atmos.d directories, remote URLs, and environment variables—and updates the processing of configuration file paths. New fields (CliConfigPath, Import) and logging sections are added, while CLI persistent flags (--base-path and --config) and environment variables (ATMOS_CLI_CONFIG_PATH, ATMOS_BASE_PATH) are now set before executing terraform and helmfile commands. Documentation, tests, vendor processing, utility functions, and dependency declarations have also been updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as Atmos CLI
participant Args as ProcessCommandLineArgs
participant Loader as ConfigLoader
participant Viper as Viper Library
participant FS as Filesystem/Remote
User->>CLI: Run Atmos command with flags (--base-path, --config, --logs-level)
CLI->>Args: Parse CLI flags and arguments
Args->>Loader: Initialize ConfigLoader with provided configAndStacksInfo
Loader->>Viper: Load schema defaults & embedded configuration
Loader->>FS: Read explicit config files (local atmos.yaml, .atmos.yaml, atmos.d/**/*)
Loader->>Loader: Process imports (local directories & remote URLs) recursively
Loader->>Viper: Deep merge configurations in lexicographical order
Loader->>CLI: Return merged configuration object
CLI->>CLI: Set environment variables (ATMOS_CLI_CONFIG_PATH, ATMOS_BASE_PATH)
CLI->>User: Execute terraform/helmfile commands with updated env vars
Assessment against linked issues
Possibly related PRs
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: 9
🧹 Outside diff range and nitpick comments (7)
examples/demo-atmos.d/atmos.yaml (3)
4-11: Document security-sensitive settingsCritical settings like
apply_auto_approveandauto_generate_backend_filewould benefit from inline documentation explaining their security implications.Consider adding comments:
components: terraform: base_path: "components/terraform" + # Disable auto-approve for safety in production environments apply_auto_approve: false deploy_run_init: true init_run_reconfigure: true + # Disable auto-generation of backend files to prevent unauthorized state access auto_generate_backend_file: false
12-19: Consider expanding name pattern flexibilityThe current name pattern only supports {stage}. Consider supporting additional variables for better organization.
Consider expanding:
- name_pattern: "{stage}" + name_pattern: "{org}-{stage}-{component}"
20-29: Clean up formatting and enhance documentationThe vendor configuration contains helpful examples but needs formatting cleanup.
Apply these changes:
-vendor: +vendor: # Single file - base_path: "./vendor.yaml" + base_path: "./vendor.yaml" # Default configuration # Directory with multiple files #base_path: "./vendor" # Absolute path #base_path: "vendor.d/vendor1.yaml"🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
examples/demo-atmos.d/custom-import/atmos.yaml (2)
26-35: Consider moving configuration examples to documentationWhile the commented examples are helpful, they might be better suited in the documentation, keeping the configuration file cleaner.
- # Single file base_path: "./vendor.yaml" - - # Directory with multiple files - #base_path: "./vendor" - - # Absolute path - #base_path: "vendor.d/vendor1.yaml"🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
36-38: Consider environment-specific log configurationsThe current logging setup is good for development, but you might want to add environment-specific configurations for production use.
Example addition:
logs: file: "${ATMOS_LOG_FILE:-/dev/stderr}" level: "${ATMOS_LOG_LEVEL:-Info}"pkg/schema/schema.go (1)
29-29: Add field documentation.Consider adding a documentation comment to describe the purpose and usage of the
Importfield, following Go's documentation conventions.Add this documentation above the field:
+ // Import specifies a list of paths from which to import additional configurations. + // Supports local files, directories (using glob patterns), and remote URLs. Import []string `yaml:"import" json:"import" mapstructure:"import"`pkg/config/config.go (1)
231-232: Combine the return statement for clarityThe return statement is unnecessarily split across two lines.
Consider writing it on a single line:
-return cliConfig, - err +return cliConfig, err
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
examples/demo-atmos.d/atmos.d/configs.d/config1.yaml(1 hunks)examples/demo-atmos.d/atmos.d/configs.d/sub/config2.yaml(1 hunks)examples/demo-atmos.d/atmos.yaml(1 hunks)examples/demo-atmos.d/custom-import/atmos.yaml(1 hunks)examples/demo-atmos.d/custom-import/configs.d/config1.yaml(1 hunks)examples/demo-atmos.d/custom-import/configs.d/sub/config2.yaml(1 hunks)examples/demo-atmos.d/custom-import/extra-config.yaml(1 hunks)pkg/config/config.go(3 hunks)pkg/schema/schema.go(1 hunks)pkg/utils/glob_utils.go(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- examples/demo-atmos.d/atmos.d/configs.d/config1.yaml
- examples/demo-atmos.d/atmos.d/configs.d/sub/config2.yaml
- examples/demo-atmos.d/custom-import/configs.d/config1.yaml
- examples/demo-atmos.d/custom-import/configs.d/sub/config2.yaml
- examples/demo-atmos.d/custom-import/extra-config.yaml
🧰 Additional context used
🪛 yamllint (1.29.0-1)
examples/demo-atmos.d/atmos.yaml
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
[warning] 38-38: wrong indentation: expected 2 but found 0
(indentation)
[warning] 41-41: wrong indentation: expected 4 but found 2
(indentation)
examples/demo-atmos.d/custom-import/atmos.yaml
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 44-44: wrong indentation: expected 2 but found 0
(indentation)
[warning] 47-47: wrong indentation: expected 4 but found 2
(indentation)
🔇 Additional comments (6)
examples/demo-atmos.d/atmos.yaml (1)
30-33: Add validation for log levels
Consider adding validation to ensure only valid log levels (Debug, Info, Warning, Error) are accepted.
examples/demo-atmos.d/custom-import/atmos.yaml (3)
7-9: Consider adding URL validation for remote configurations
While loading from remote URLs provides flexibility, it could pose security risks. Consider implementing URL validation and HTTPS enforcement.
Would you like me to propose a validation pattern for secure URL handling?
18-24: Strong stack configuration, warrior! 💪
The stack configuration is well-structured with clear inclusion/exclusion patterns and a simple naming convention.
11-16: Verify backend file management strategy
Setting auto_generate_backend_file: false means you'll need to manage backend configurations manually. This is a valid choice but requires careful consideration.
Run this check to ensure consistent backend configuration across components:
✅ Verification successful
Setting auto_generate_backend_file: false is appropriate for this setup
The configuration choice is valid as there's no existing Terraform component directory at the specified path, indicating this is likely a new project setup. Manual backend management is a reasonable approach when starting fresh, allowing for more controlled and explicit backend configuration as the project evolves.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing backend configurations
fd -e tf -e hcl . components/terraform -x grep -l 'backend'
Length of output: 167
Script:
#!/bin/bash
# First, let's verify if the components/terraform directory exists and check the directory structure
fd --type d terraform
# Then, let's search for backend configurations across all Terraform files in the repository
fd -e tf -e hcl . -x grep -l 'backend'
# Also check for any Terraform component references in the atmos configuration
fd -g "*.yaml" -x grep -A 5 -B 5 "terraform:"
Length of output: 33961
Script:
#!/bin/bash
# Let's check for backend configurations in the actual Terraform components
fd -t f -e tf . examples/demo-atmos.d/custom-import/components/terraform -x grep -l 'backend'
# Also check for any backend configuration files
fd -t f -g "*.backend.tf" -g "backend.tf" examples/demo-atmos.d/custom-import/components/terraform
# Check if there are any existing backend configurations in the component directory
ls -la examples/demo-atmos.d/custom-import/components/terraform/
Length of output: 709
pkg/utils/glob_utils.go (2)
7-7: LGTM! Clean import addition, warrior!
The sort package import is well-placed and necessary for the new functionality.
Line range hint 19-48: Verify caching behavior for dynamic configurations, brave one!
The caching mechanism using sync.Map could prevent picking up new configuration files added to atmos.d during runtime. Additionally, we should verify that the error handling aligns with the requirement to log non-existent paths as warnings.
Let's verify the implementation:
✅ Verification successful
Cache invalidation is not a concern, brave warrior!
The caching mechanism in GetGlobMatches is pattern-based, which means:
- Each unique glob pattern gets its own cache entry
- When a new file is added that matches an existing pattern, it will be discovered on the next process restart
- The cache is process-scoped (using
sync.Map), so it's automatically cleared between runs - Error handling correctly returns both the error and logs the non-existent paths
The implementation aligns perfectly with the requirements:
- Cache is scoped to the current process, preventing stale state between runs
- Error handling returns descriptive errors for non-existent paths:
failed to find a match for the import '%s' ('%s' + '%s')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for warning logs related to non-existent paths and cache invalidation
# Search for warning log implementations
rg -A 2 "failed to find a match for the import" .
# Check if there's any cache invalidation mechanism
rg -A 5 "getGlobMatchesSyncMap" .
# Look for any warning level log statements
rg -A 2 "Warning|WARN|warn" .
Length of output: 47400
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
pkg/config/config.go(3 hunks)pkg/schema/schema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schema/schema.go
🔇 Additional comments (1)
pkg/config/config.go (1)
231-232: LGTM! Clean error handling.
The multi-line error return improves readability.
…correct command structure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
examples/demo-atmos.d/custom-import/atmos.yaml (2)
26-34: Clean up trailing spacesRemove trailing spaces from lines 26, 29, and 32 for better maintainability.
-vendor: +vendor: # Single file -base_path: "./vendor.yaml" +base_path: "./vendor.yaml" # Directory with multiple files -#base_path: "./vendor" +#base_path: "./vendor"🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
36-38: Logging configuration is good but could be enhancedCurrent configuration is solid. Consider adding:
- Log rotation settings
- Format specification
- Additional log destinations for production environments
pkg/config/config.go (1)
401-444: Consider adding rate limiting and path validationThe import processing is robust, but consider these enhancements:
- Add rate limiting for remote downloads to prevent abuse
- Validate import paths before processing to ensure they're safe
func processImports(cliConfig schema.CliConfiguration, v *viper.Viper) error { + // Add rate limiting for remote downloads + rateLimiter := time.NewTicker(time.Second) + defer rateLimiter.Stop() + for _, importPath := range cliConfig.Import { if importPath == "" { continue } + // Basic path validation + if strings.Contains(importPath, "..") { + u.LogWarning(cliConfig, fmt.Sprintf("Warning: skipping potentially unsafe import path '%s'", importPath)) + continue + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
examples/demo-atmos.d/atmos.d/config1.yaml(1 hunks)examples/demo-atmos.d/atmos.d/sub/config2.yaml(1 hunks)examples/demo-atmos.d/custom-import/atmos.yaml(1 hunks)pkg/config/config.go(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/demo-atmos.d/atmos.d/config1.yaml
- examples/demo-atmos.d/atmos.d/sub/config2.yaml
🧰 Additional context used
🪛 yamllint (1.29.0-1)
examples/demo-atmos.d/custom-import/atmos.yaml
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
examples/demo-atmos.d/custom-import/atmos.yaml (4)
11-16: Terraform security settings look good!
The Terraform component configuration follows security best practices:
apply_auto_approve: falseprevents unintended infrastructure changesauto_generate_backend_file: falseensures explicit backend configuration
18-24: Stack configuration structure is well-organized!
Good use of:
- Explicit path inclusions/exclusions
- Clear naming pattern
- Logical directory structure
43-47: Command configuration looks good!
The test command is well-defined with clear description and steps.
7-9: Verify security measures for remote configuration imports
The remote URL import capability could pose security risks. Ensure that:
- HTTPS is enforced for remote URLs
- URL validation is implemented
- Content verification is performed before applying configurations
Consider implementing:
- URL allowlist/blocklist
- SSL certificate validation
- Content integrity checks (e.g., checksums)
pkg/config/config.go (3)
231-232: Clean error handling format!
The multi-line error return improves readability while maintaining functionality.
239-250: Validate path to prevent directory traversal
While the implementation correctly handles the atmos.d directory existence, consider adding path validation as suggested in the past review to prevent potential path traversal attacks.
446-464: Add safeguards to remote downloads
The download function needs the safeguards mentioned in the past review:
- Context timeout
- File size limits
- Temporary directory cleanup
…onfig and processImports
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
pkg/config/config.go (4)
231-232: Consider using idiomatic Go error handling.While splitting the return statement improves readability, consider using the more idiomatic Go style:
- return cliConfig, - err + return cliConfig, fmt.Errorf("failed to unmarshal config: %w", err)
239-256: Strong implementation with robust security measures!The atmos.d directory support is well-implemented with proper path validation and error handling. The path traversal prevention is particularly noteworthy.
Consider adding more context to the error message:
- return cliConfig, fmt.Errorf("invalid atmos.d path: attempted directory traversal") + return cliConfig, fmt.Errorf("invalid atmos.d path '%s': attempted directory traversal outside base path '%s'", atmosDPath, basePath)
257-269: Consider adding debug logging for configuration merging.The import processing logic is sound, but adding debug logs would help troubleshoot configuration issues.
err = v.Unmarshal(&cliConfig) if err != nil { + u.LogTrace(cliConfig, fmt.Sprintf("Failed to unmarshal merged config: %v", err)) return cliConfig, err } + u.LogTrace(cliConfig, "Successfully merged and unmarshaled imported configurations")
407-459: Consider parallel processing for multiple imports.The implementation is robust with proper error handling and path validation. However, processing imports sequentially might be slow for many imports.
Consider using a worker pool for parallel processing:
func processImports(cliConfig schema.CliConfiguration, v *viper.Viper) error { + type importResult struct { + path string + err error + } + workers := make(chan struct{}, 4) // Limit concurrent downloads + results := make(chan importResult) + for _, importPath := range cliConfig.Import { - if importPath == "" { - continue - } + go func(imp string) { + workers <- struct{}{} // Acquire worker + defer func() { <-workers }() // Release worker + + if imp == "" { + results <- importResult{"", nil} + return + } + // ... rest of the import processing logic ... + }(importPath) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/config/config.go(3 hunks)
🔇 Additional comments (1)
pkg/config/config.go (1)
461-479: Past review comment about remote download safeguards is still applicable.
The previous review comment suggesting context timeout, file size limits, and temporary directory cleanup remains valid.
…andle cleanup on error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/config/config.go (1)
232-233: Combine the return statement into a single lineThe return statement is split across two lines. Merging it into one line enhances readability.
Apply this diff:
- return cliConfig, - err + return cliConfig, err
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/config/config.go(3 hunks)
🔇 Additional comments (4)
pkg/config/config.go (4)
240-257: Good handling of the 'import' key and path security
The logic for setting the default Import path when it's empty is well-implemented. Ensuring the path doesn't escape the intended directory adds a valuable security check.
259-270: Import processing added effectively
The addition of import processing and re-unmarshalling is correctly handled. Error checks are appropriately placed.
417-426: Consider validating remote URLs for security
Currently, any URL starting with "http://" or "https://" is accepted. Validating these URLs against an allowlist can prevent unintended or malicious imports.
464-484: Add a file size limit check in downloadRemoteConfig
While the context timeout is set, the function lacks a file size limit check. This could allow the download of excessively large files, leading to resource exhaustion.
|
Update documentation for Atmos CLI Configuration schema and examples |
|
Update documentation for Atmos CLI Configuration |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/config/base_path.go (3)
13-13: Refine docstring wording.Line 13 mentions "Derive the effective base-base," which appears to be a minor typographical duplication. Consider changing it to "Derive the effective base path."
-// Derive the effective base-base based on multiple inferences +// Derive the effective base path based on multiple inferences
74-74: Enhance logging consistency.Here, the call to
log.Debugpasses multiple arguments: the string literal "base path derived from," thensource, thenpath. Consider using a more structured approach to improve clarity and keep it consistent with other log statements in the code.-log.Debug("base path derived from", source, path) +log.Debug("base path derived from "+source, "path", path)
87-135: Suggest adding tests to confirm the correctness of inferred base paths.The logic spans multiple checks (
checkConfigFileOrDir, fallback to.atmos, fallback toatmos.d, fallback to git root), all of which influence the final base path. Consider adding or expanding tests to validate each scenario, ensuring the correct path is selected.Would you like me to provide an outline of these test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/base_path.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/base_path.go (2)
45-45: Avoid referencing “infra” in this logging message.Past feedback indicated there is no concept of “infra” in Atmos. Replace “infra” with a more suitable term to maintain clarity and consistency.
-log.Debug("base path derived from infra", "base_path", absPath) +log.Debug("base path derived automatically", "base_path", absPath)
104-104: Pass thecwdargument when callingGetGitRoot().A past review comment noted that
GetGitRoot()requires astartPathparameter. Omitting it may lead to unexpected results.- gitTopLevel, err := u.GetGitRoot() + gitTopLevel, err := u.GetGitRoot(cwd)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/config/base_path.go (1)
105-105:⚠️ Potential issueFix GetGitRoot function call.
The GetGitRoot function requires a startPath parameter.
- gitTopLevel, err := u.GetGitRoot() + gitTopLevel, err := u.GetGitRoot(cwd)
🧹 Nitpick comments (3)
pkg/config/loader.go (2)
31-35: Consider adding mutex protection for thread safety.The
ConfigLoaderstruct contains shared state that could be accessed concurrently. Consider adding a mutex to protect access to shared fields.type ConfigLoader struct { + mu sync.RWMutex viper *viper.Viper atmosConfig schema.AtmosConfiguration AtmosConfigPaths []string }
532-534: Remove redundant return statement.The return statement on line 533 is redundant as it's at the end of the function.
log.Debug("no configuration found in user preferences") - return🧰 Tools
🪛 golangci-lint (1.62.2)
533-533: S1023: redundant
returnstatement(gosimple)
pkg/config/base_path.go (1)
118-121: Optimize glob pattern matching.The code performs two separate glob operations for .yaml and .yml files. This can be optimized into a single operation.
- filePaths, _ := u.GetGlobMatches(filepath.ToSlash(filepath.Join(path, "**/*.yaml"))) - if len(filePaths) == 0 { - filePaths, _ = u.GetGlobMatches(filepath.ToSlash(filepath.Join(path, "**/*.yml"))) - } + filePaths, _ := u.GetGlobMatches(filepath.ToSlash(filepath.Join(path, "**/*.{yaml,yml}")))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/config/base_path.go(1 hunks)pkg/config/imports.go(1 hunks)pkg/config/loader.go(1 hunks)pkg/config/loader_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/config/loader_test.go
- pkg/config/imports.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/loader.go
533-533: S1023: redundant return statement
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/config/loader.go (2)
47-93: LGTM! Well-structured configuration loading flow.The method follows a clear and logical flow for loading configurations from various sources. The error handling pattern allows for resilient partial configuration loading, which is the desired behavior.
603-624: LGTM! Well-implemented search functionality.The
SearchAtmosConfigmethod is well-structured with clear separation of concerns into helper functions. The pattern handling and file prioritization logic is robust.
| func isRemoteImport(importPath string) bool { | ||
| return strings.HasPrefix(importPath, "http://") || strings.HasPrefix(importPath, "https://") | ||
| } |
There was a problem hiding this comment.
Use viper native remote config handling
| } | ||
|
|
||
| // Process remote imports | ||
| func (cl *ConfigLoader) processRemoteImport(importPath, tempDir string, currentDepth, maxDepth int) ([]ResolvedPaths, error) { |
There was a problem hiding this comment.
Use viper native remote config handling
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
| BasePathFromArg string | ||
| AtmosConfigPathFromArg []string |
There was a problem hiding this comment.
This seems odd to me. Why are we circumbenting cobra? We should ask cobra if it's set, not create another flag somewhere.
There was a problem hiding this comment.
yes its set from Cobra on ProcessCommandLineArgs functions
| func (cl *ConfigLoader) resolveAndValidatePath(path string, source string) (string, error) { | ||
| absPath, err := filepath.Abs(path) | ||
| if err != nil { | ||
| return "", fmt.Errorf("base path from %s does not exist: %w", source, err) |
There was a problem hiding this comment.
This function is resolveAndValidatePath and doesn't indicate any relationship to base path. Shouldn't thisbe more generic?
| } | ||
|
|
||
| // Deep Merge Schema Defaults and Embedded Config | ||
| err = cl.deepMergeConfig() |
There was a problem hiding this comment.
Viper supports native deep merging
There was a problem hiding this comment.
I using viper.Unmarshal(&cl.atmosConfig)
| configPath, found := cl.SearchConfigFilePath(searchFilePath) | ||
| if found { | ||
| atmosFoundFilePaths = append(atmosFoundFilePaths, configPath) | ||
| } else { | ||
| log.Debug("Failed to find config", "path", atmosCliConfigPathEnv) | ||
| } | ||
| searchDir := filepath.Join(filepath.FromSlash(atmosCliConfigPathEnv), "atmos.d/**/*") | ||
| foundPaths, err := cl.SearchAtmosConfig(searchDir) |
There was a problem hiding this comment.
What's the difference between SearchConfigFilePath and SearchAtmosConfig, and why do we need 2 functions?
There was a problem hiding this comment.
SearchConfigFilePath return one file .yaml or .yml from path
SearchAtmosConfig return many files with exactions yaml ,yml from path
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
pkg/config/base_path_test.go (2)
31-31: Fix typo in test comment.- // test base pat not directory + // test base path not directory
12-45: Consider using table-driven tests for better maintainability.The test cases follow similar patterns and could be refactored into a table-driven test structure. This would:
- Reduce code duplication
- Make it easier to add new test cases
- Improve maintainability
Example structure:
func TestComputeBasePath(t *testing.T) { tests := []struct { name string setup func() (schema.ConfigAndStacksInfo, string, func()) expectedError bool }{ { name: "valid base path arg", setup: func() (schema.ConfigAndStacksInfo, string, func()) { return schema.ConfigAndStacksInfo{BasePathFromArg: "."}, ".", nil }, expectedError: false, }, // Add more test cases } // Test execution loop }Also applies to: 47-65, 67-110
pkg/config/base_path.go (1)
44-44: Update log message to be more descriptive.- log.Debug("base path derived from infra", "base_path", absPath) + log.Debug("base path inferred", "source", "directory scan", "path", absPath)pkg/utils/yaml_utils.go (1)
175-177: Move Git-related functionality to git.go.Based on past review comments, Git-related functionality should be consolidated in git.go for better code organization.
Consider moving this logic to a new function in git.go:
// git.go func GetGitRootForYAML() (string, error) { return GetGitRoot() }Then update this code to:
if tag == AtmosYamlFuncGitRoot { - return GetGitRoot() + return GetGitRootForYAML() }pkg/config/config.go (1)
119-143: Consider simplifying the default config loading logic.The nested if conditions and error handling could be simplified for better readability.
Consider this structure:
if len(configLoader.AtmosConfigPaths) == 0 { + if shouldLogDebug := logsLevelEnvVar == u.LogLevelDebug || logsLevelEnvVar == u.LogLevelTrace; shouldLogDebug { + u.PrintMessageInColor("'atmos.yaml' CLI config was not found...", theme.Colors.Info) + if err := u.PrintAsYAMLToFileDescriptor(atmosConfig, defaultCliConfig); err != nil { + return atmosConfig, err + } + fmt.Println() + } + + // Load Atmos Schema Defaults + if j, err := json.Marshal(defaultCliConfig); err != nil { + return atmosConfig, err + } else if err := configLoader.viper.MergeConfig(bytes.NewReader(j)); err != nil { + return atmosConfig, err + } }pkg/config/loader.go (4)
31-35: Add documentation for exported fields.The
ConfigLoaderstruct has exported fields but lacks documentation. Consider adding comments to explain their purpose.type ConfigLoader struct { + // viper is the underlying configuration manager viper *viper.Viper + // atmosConfig holds the current configuration state atmosConfig schema.AtmosConfiguration + // AtmosConfigPaths stores the paths of loaded configuration files AtmosConfigPaths []string }
47-93: Consider breaking down the LoadConfig method.The method handles multiple stages of configuration loading. Consider extracting each stage into a separate private method for better maintainability.
Example structure:
func (cl *ConfigLoader) LoadConfig(configAndStacksInfo schema.ConfigAndStacksInfo) (schema.AtmosConfiguration, error) { if err := cl.initializeDefaults(); err != nil { return cl.atmosConfig, err } if err := cl.loadEmbeddedAndSchemaDefaults(); err != nil { return cl.atmosConfig, err } if err := cl.handleExplicitConfigs(configAndStacksInfo); err != nil { return cl.atmosConfig, err } // ... continue with other stages return cl.atmosConfig, nil }
834-864: Improve error handling in import processing.The error handling in the import processing could be more robust.
Consider:
- Adding context to errors
- Handling multiple import failures gracefully
func (cl *ConfigLoader) processConfigImports() error { if len(cl.atmosConfig.Import) > 0 { tempDir, err := os.MkdirTemp("", "atmos-import-*") if err != nil { - return err + return fmt.Errorf("failed to create temporary directory for imports: %w", err) } defer os.RemoveAll(tempDir) resolvedPaths, err := cl.processImports(cl.atmosConfig.Import, tempDir, 1, MaximumImportLvL) if err != nil { - return err + return fmt.Errorf("failed to process imports: %w", err) }
538-538: Remove redundant return statement.The return statement at the end of the function is unnecessary.
- return🧰 Tools
🪛 golangci-lint (1.62.2)
538-538: S1023: redundant
returnstatement(gosimple)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/config/base_path.go(1 hunks)pkg/config/base_path_test.go(1 hunks)pkg/config/config.go(4 hunks)pkg/config/imports.go(1 hunks)pkg/config/loader.go(1 hunks)pkg/utils/git.go(1 hunks)pkg/utils/yaml_utils.go(3 hunks)tests/cli_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/imports.go
🧰 Additional context used
🧠 Learnings (1)
pkg/config/config.go (2)
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Learnt from: osterman
PR: cloudposse/atmos#887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
🪛 golangci-lint (1.62.2)
pkg/config/loader.go
538-538: S1023: redundant return statement
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/utils/git.go (1)
11-35: LGTM! The Git root detection implementation is robust.The function effectively uses go-git to find the repository root and handles errors appropriately. The implementation includes:
- Proper error handling with descriptive messages
- Path resolution using filepath.Abs
- Detection of .git directory using DetectDotGit option
pkg/config/base_path.go (1)
104-104: Verify GetGitRoot function call.Based on past review comments, the GetGitRoot function requires a startPath parameter.
Run this script to check the function signature:
❌ Incorrect review comment
🏁 Script executed:
#!/bin/bash ast-grep --pattern 'func GetGitRoot($$$) $$$'Length of output: 1267
Clarification: GetGitRoot Signature Is Correct as Implemented
The GetGitRoot function in
pkg/utils/git.gois defined without any parameters, so the call inpkg/config/base_path.gois correct. The review comment appears to reference a past expectation that required a startPath parameter, but the current implementation gathers the working directory internally. No changes are needed.Likely an incorrect or invalid review comment.
pkg/config/config.go (3)
114-115: LGTM! Good modularization.The introduction of ConfigLoader improves code organization and maintainability by encapsulating configuration loading logic.
144-149: LGTM! Good error handling.The error handling for base path computation is clear and informative.
271-295: LGTM! Improved error handling pattern.The updated function signature with
(bool, string, error)return values provides better context and error handling capabilities.tests/cli_test.go (1)
587-613: LGTM! Enhanced output verification.The addition of output sanitization before verification improves test reliability and error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/config/loader.go (3)
25-25: Add documentation for MaximumImportLvL constant.Add a comment explaining the purpose of this constant and why it's set to 10.
+// MaximumImportLvL defines the maximum depth level for recursive configuration imports +// to prevent infinite recursion. The value is set to 10 as a reasonable limit. const MaximumImportLvL = 10
539-539: Remove redundant return statement.The return statement is redundant as the function already returns at the end.
- return🧰 Tools
🪛 golangci-lint (1.62.2)
539-539: S1023: redundant
returnstatement(gosimple)
471-471: Improve error message clarity.The error message could be more descriptive.
- return nil, fmt.Errorf("Failed to find config files in git root path %v", gitRootDir) + return nil, fmt.Errorf("no configuration files found in git repository root directory: %s", gitRootDir)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/loader.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/loader.go
539-539: S1023: redundant return statement
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
| if len(gitAtmosConfigPath) > 0 { | ||
| log.Debug("no atmos config found on Git root", "path", gitRoot, "error", err) | ||
| return found, gitRoot | ||
| } |
There was a problem hiding this comment.
Fix incorrect condition in loadGitAtmosConfig.
The condition is checking if the slice is empty (len > 0) but then logs that no config was found. This is logically incorrect.
- if len(gitAtmosConfigPath) > 0 {
- log.Debug("no atmos config found on Git root", "path", gitRoot, "error", err)
- return found, gitRoot
- }
+ if len(gitAtmosConfigPath) == 0 {
+ log.Debug("no atmos config found on Git root", "path", gitRoot)
+ return found, gitRoot
+ }📝 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 len(gitAtmosConfigPath) > 0 { | |
| log.Debug("no atmos config found on Git root", "path", gitRoot, "error", err) | |
| return found, gitRoot | |
| } | |
| if len(gitAtmosConfigPath) == 0 { | |
| log.Debug("no atmos config found on Git root", "path", gitRoot) | |
| return found, gitRoot | |
| } |
what
atmos.dconvention foratmos.yamlconfiguration, allowing automatic inclusion of configuration files from the atmos.d directory.importkey inside `atmos.yaml, allowing users to define a list of locations (local files, directories using glob patterns, and remote URLs) to import configurations from.ATMOS_CLI_CONFIG_PATHandATMOS_BASE_PATHbefore running terraform and helm cmdexamples/demo-envthat shows exposed env variablesexamples/demo-atmos-cli-importsfor custom inclusion of configuration files from the atmos.d directoryexamples/demo-atmos.dfor automatic inclusion of configuration files from the atmos.d directorywhy
Simplifies configuration management, enabling overrides at multiple levels of processing, making it easier to include additional configurations without explicit declarations.
Details
Import Config
flowchart TD A["Load Configuration File"] --> B{"Import Section Exists?"} B -- Yes --> C["Process Imports in Order"] C --> D{"Import Type?"} D --> E["Remote URL"] D --> F["Specific Path"] D --> G["Wildcard Globs"] E --> H["Fetch Config from Remote URL"] F --> I["Read Config from Filesystem"] G --> I["Read Config from Filesystem"] H --> J["Call Load Configuration File (Recursively)"] I --> J["Call Load Configuration File (Recursively)"] J --> L["Deep Merge with Current Config in Memory"] L --> K{"More Imports to Process?"} K -- Yes --> C K -- No --> M["Configuration Processing Complete"] %% Loopback for recursion J -.-> A %% Styling for clarity style A fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style B fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style C fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style D fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style E fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style F fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style G fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style H fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style I fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style J fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style L fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M fill:#1D3557,stroke:#1D3557,stroke-width:2px,color:#FFFFFF classDef recursion stroke-dasharray: 5 5;Load Config
--- config: layout: fixed --- flowchart TD A["Start Configuration Process"] --> Z1["Load Atmos Schema Defaults"] Z1 --> Z2["Load // go:embed cloudposse/atmos/config/atmos.yaml"] Z2 --> Z3["Deep Merge Schema Defaults and Embedded Config"] Z3 --> B{"Is --config Provided?"} %% If --config is provided B -- Yes --> C["Stage 1: Load Explicit Configurations via --config"] C --> C1{"Load each --config path in order ."} C1 --> C2{"if path is dir search (/**/*yaml , /**/*yml ) in order . on same directory extension .yaml has priority over yml"} C2 --> C3["Update ATMOS_CLI_CONFIG_PATH with loaded config absolute paths (separated by delimiter)"] C3 --> D["Process Imports and Deep Merge"] D --> E["Final Merged Config"] E --> F["Output Final Configuration"] %% If --config is not provided B -- No --> G["Stage 1: Load System Configurations"] G --> G1{"Check System Paths"} G1 -- Found --> G2["Load First Found Config: %PROGRAMDATA%/atmos.yaml (Windows), /usr/local/etc/atmos.yaml, or /etc/atmos.yaml"] G2 --> G3["Update ATMOS_CLI_CONFIG_PATH"] G3 --> H["Process Imports and Deep Merge"] G1 -- Not Found --> H["Process Imports and Deep Merge"] H --> I["Stage 2: Discover Additional Configurations"] I --> I1{"Check ATMOS_CLI_CONFIG_PATH"} I1 -- Found --> I2["Load First Found Config: atmos.yaml or atmos.d/**/* from ATMOS_CLI_CONFIG_PATH"] I2 --> I4["Update ATMOS_CLI_CONFIG_PATH with loaded absolute paths"] I4 --> J["Process Imports and Deep Merge"] I1 -- Not Found --> I12{"Check Current Working Directory"} %% New branch for Current Working Directory (note it's not identical to repo root) I12 -- Found --> I13["Load First Found Config: atmos.yaml, .atmos.yaml, atmos.d/**/*, .atmos.d/**/* from CWD"] I13 --> I4 I12 -- Not Found --> I5{"Check Git Repository Root"} I5 -- Found --> I6["Load First Found Config: atmos.yaml, .atmos.yaml, atmos.d/**/*, .atmos.d/**/*, or .github/atmos.yaml from Git Repo Root"] I6 --> I4 I5 -- Not Found --> I18["No configuration found in Stage 2"] I18 --> K["Stage 3: Apply User Preferences"] J --> K["Stage 3: Apply User Preferences"] K --> K1{"Check $XDG_CONFIG_HOME/atmos.yaml"} K1 -- Found --> K2["Load First Found Config: $XDG_CONFIG_HOME/atmos.yaml"] K2 --> K3["Update ATMOS_CLI_CONFIG_PATH with $XDG_CONFIG_HOME/atmos.yaml"] K3 --> L["Process Imports and Deep Merge"] K1 -- Not Found --> K4{"Check User's Home Directory"} K4 -- Found --> K5["Load First Found Config: %LOCALAPPDATA%/atmos/atmos.yaml (Windows), ~/.config/atmos/atmos.yaml, or ~/.atmos/atmos.yaml (Linux/macOS)"] K5 --> K3 K4 -- Not Found --> K7["No configuration found in Stage 3"] K7 --> M["Final Merged Config"] L --> M["Final Merged Config"] M --> F["Output Final Configuration"] %% Styling for clarity style A fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style Z1 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style Z2 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style Z3 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style B fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style C fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style C1 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style C2 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style D fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style E fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style F fill:#1D3557,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style G fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style G1 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style G2 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style G3 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style H fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I1 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I2 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I4 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style J fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I5 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I6 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I12 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I13 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I18 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style K fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K1 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K2 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K3 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style K4 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K5 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K7 fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style L fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFFBase Path
flowchart TD A["Load Atmos Configuration"] --> B["Start Computing Base Path"] B --> C{"Is --base-path Provided?"} %% If --base-path is provided C -- Yes --> D["Set base_path to --base-path (resolve absolute if relative)"] D --> E["Update ATMOS_BASE_PATH with absolute path"] %% If --base-path is not provided C -- No --> G{"Is ATMOS_BASE_PATH Set?"} G -- Yes --> H["Set base_path to ATMOS_BASE_PATH (resolve absolute if relative)"] H --> E G -- No --> I{"Is base_path Set in Configuration?"} I -- Yes --> J{"Is base_path Absolute?"} J -- Yes --> K["Set base_path as is"] K --> E J -- No --> L["Resolve base_path relative to current working directory"] L --> K I -- No --> M["Infer base_path"] M --> M1{"Check CWD for atmos.yaml, .atmos.yaml, atmos.d/**/*, .github/atmos.yaml"} M1 -- Found --> M2["Set base_path to absolute path containing directory"] M1 -- Not Found --> M3{"Is Git Repository Detected?"} M3 -- Yes --> M4["Set base_path to Git Repository Root"] M3 -- No --> M5["Set base_path to absolute path of ./"] M2 --> E M4 --> E M5 --> E E --> F["Base Path Computed"] %% Styling for clarity style A fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style B fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style C fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style D fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style E fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style F fill:#457B9D,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style G fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style H fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style I fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style J fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style K fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style L fill:#A8DADC,stroke:#1D3557,stroke-width:2px,color:#000000 style M fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M1 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M2 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M3 fill:#F4A261,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M4 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFF style M5 fill:#E63946,stroke:#1D3557,stroke-width:2px,color:#FFFFFFreferences
Summary by CodeRabbit
New Features
--base-pathand--config) to let users explicitly define project paths and configuration files.Documentation
atmos.yamlto reflect changes in configuration structure and loading precedence.Enhancements