Refine Atmos Configuration Handling to Support Imports, Remote Imports, and Configuration Directories #808
Conversation
haitham911
requested review from
johncblandii,
RoseSecurity,
aknysh and
osterman
📝 WalkthroughPull Request AnalysisWalkthroughThis pull request introduces a comprehensive overhaul of the Atmos CLI configuration loading system. The changes centralize configuration management by implementing a new Changes
Assessment against linked issues
Possibly related PRs
Suggested ReviewersBy Zeus's beard, this configuration loading system is a testament to engineering prowess! The implementation provides a robust, flexible approach to managing complex configuration scenarios, much like a well-organized Spartan phalanx adapts to battlefield conditions. Each configuration source is carefully considered, merged with precision, and handled with strategic error management. The new system supports importing configurations from multiple sources - local files, directories, and even remote URLs - while maintaining a clear precedence and merge strategy. It's not just a configuration loader; it's a configuration conquest! Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ 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 (4)
pkg/config/con.go (4)
25-25: Follow Go naming conventions for constants.The constant
MaximumImportLvLshould be renamed tomaximumImportLevelormaxImportLevelfor better readability and consistency with Go naming conventions.-const MaximumImportLvL = 10 +const maxImportLevel = 10🧰 Tools
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
693-697: Useerrors.Newfor static error messages.When creating an error with a static message, use
errors.Newinstead offmt.Errorf.- errorMessage := fmt.Sprintf("\nno stack manifests found in the provided "+ - "paths:\n%s\n\nCheck if `base_path`, 'stacks.base_path', 'stacks.included_paths' and 'stacks.excluded_paths' are correctly set in CLI config "+ - "files or ENV vars.", j) - return fmt.Errorf(errorMessage) + return fmt.Errorf("\nno stack manifests found in the provided paths:\n%s\n\nCheck if `base_path`, 'stacks.base_path', 'stacks.included_paths' and 'stacks.excluded_paths' are correctly set in CLI config files or ENV vars", j)🧰 Tools
🪛 golangci-lint (1.62.2)
696-696: printf: non-constant format string in call to fmt.Errorf
(govet)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
906-922: Remove unused functionfindGitTopLevel.The function
findGitTopLevelis not used anywhere in the codebase. Consider removing it or documenting why it's kept for future use.🧰 Tools
🪛 golangci-lint (1.62.2)
906-906: func
findGitTopLevelis unused(unused)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
874-874: Use proper string formatting in warning message.Replace string concatenation with proper formatting using
fmt.Sprintf.- u.LogWarning(atmosConfig, fmt.Sprintf("error closing file '"+path+"'. "+err.Error())) + u.LogWarning(atmosConfig, fmt.Sprintf("error closing file '%s': %v", path, err))🧰 Tools
🪛 golangci-lint (1.62.2)
874-874: printf: non-constant format string in call to fmt.Sprintf
(govet)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/config/base_path.go(1 hunks)pkg/config/con.go(1 hunks)pkg/config/imports.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/config/base_path.go
- pkg/config/imports.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/con.go
532-532: Error return value of cl.deepMergeConfig is not checked
(errcheck)
906-906: func findGitTopLevel is unused
(unused)
696-696: printf: non-constant format string in call to fmt.Errorf
(govet)
874-874: printf: non-constant format string in call to fmt.Sprintf
(govet)
809-809: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
🪛 GitHub Actions: Tests
pkg/config/con.go
[error] Invalid file path detected during git checkout process
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/config/con.go (2)
532-532:⚠️ Potential issueHandle the error returned by
deepMergeConfig.The error returned by
deepMergeConfigshould be checked to ensure proper error handling.- cl.deepMergeConfig() + if err := cl.deepMergeConfig(); err != nil { + return err + }Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
532-532: Error return value of
cl.deepMergeConfigis not checked(errcheck)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
809-809:⚠️ Potential issueAddress unused HasSuffix result.
The result of
strings.HasSuffixis computed but not used. This might indicate a logical error.- } else if ext == ".yml" && strings.HasSuffix(existingFile, ".yaml") { + } else if ext == ".yml" { + // If you need to check for .yaml suffix, use the result + if strings.HasSuffix(existingFile, ".yaml") { + continue // Skip if it's a .yml file but has .yaml suffix + }Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
809-809: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/config/base_path_test.go (3)
33-33: Fix typo in comment.- // test base pat not directory + // test base path not directory
70-70: Fix typo and formatting in comment.-// test base_path Set in Configuration +// test base_path set in configuration
118-118: Fix typo in comment.-// test base path form "!repo-root" +// test base path from "!repo-root"pkg/config/base_path.go (1)
26-26: Remove duplicate comment.- // Check base path from configurationpkg/config/con.go (1)
874-874: Simplify string formatting.No need to use
fmt.Sprintfwhen the argument is already a string.- u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) + u.LogWarning(atmosConfig, fmt.Sprintf("error closing file '%s': %v", path, err))🧰 Tools
🪛 golangci-lint (1.62.2)
874-874: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/config/base_path.go(1 hunks)pkg/config/base_path_test.go(1 hunks)pkg/config/con.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/base_path_test.go
28-28: ineffectual assignment to expectedPath
(ineffassign)
pkg/config/con.go
532-532: Error return value of cl.deepMergeConfig is not checked
(errcheck)
906-906: func findGitTopLevel is unused
(unused)
874-874: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
696-696: printf: non-constant format string in call to fmt.Errorf
(govet)
809-809: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
🪛 GitHub Actions: Tests
pkg/config/con.go
[error] Invalid file path detected during git checkout process
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/config/base_path_test.go (2)
13-48: Well-structured test cases covering all scenarios!The test suite comprehensively covers various base path computation scenarios:
- Base path from CLI argument
- Base path from environment variable
- Base path from configuration
- Base path from repo root
Good job on including both positive and negative test cases, and proper cleanup using
defer.Also applies to: 51-68, 71-116, 119-134
🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: ineffectual assignment to expectedPath
(ineffassign)
89-92:⚠️ Potential issueReplace os.Exit(1) with t.Fatal in test.
Using
os.Exit(1)in tests is not recommended as it prevents proper test cleanup and reporting.Apply this diff to fix the issue:
- fmt.Printf("Failed to get the current working directory: %v\n", err) - os.Exit(1) // Exit with a non-zero code to indicate failure + t.Fatalf("Failed to get the current working directory: %v", err)Likely invalid or redundant comment.
pkg/config/base_path.go (3)
13-72: Well-structured base path resolution with clear priority!The
BasePathComputingfunction implements a clear priority order for resolving the base path:
- CLI argument
- Environment variable
- Configuration
- Inferred path
Good job on the comprehensive error handling and path validation.
133-152: Clean Git root resolution implementation!The
GetGitRootfunction:
- Uses proper error wrapping
- Provides clear error messages
- Returns absolute paths
123-124:⚠️ Potential issueFix incorrect variable in logging statement.
The logging statement uses
filePathwhich is not updated in this code path. It should usedirAbsinstead.- cl.debugLogging(fmt.Sprintf("base path from infra %s: %s", "git root", filePath)) + cl.debugLogging(fmt.Sprintf("base path from infra %s: %s", "git root", dirAbs))Likely invalid or redundant comment.
pkg/config/con.go (4)
48-144: Well-structured configuration loading process!The
LoadConfigfunction implements a clear and systematic approach to loading configurations:
- Schema defaults
- Embedded config
- Explicit configs
- System configs
- User preferences
- Additional configs
Good job on the comprehensive error handling and configuration merging strategy.
🧰 Tools
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
696-696:⚠️ Potential issueUse errors.New for static error messages.
Using
fmt.Errorfwith a non-constant format string could lead to issues if the string contains format verbs.- return fmt.Errorf(errorMessage) + return errors.New(errorMessage)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
696-696: printf: non-constant format string in call to fmt.Errorf
(govet)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
532-532:⚠️ Potential issueCheck error returned by deepMergeConfig.
The error from
deepMergeConfigshould be checked and handled appropriately.- cl.deepMergeConfig() + if err := cl.deepMergeConfig(); err != nil { + return fmt.Errorf("failed to merge config after imports: %w", err) + }Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
532-532: Error return value of
cl.deepMergeConfigis not checked(errcheck)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
809-809:⚠️ Potential issueUse HasSuffix result in condition.
The result of
HasSuffixis computed but not used.- } else if ext == ".yml" && strings.HasSuffix(existingFile, ".yaml") { + } else if ext == ".yml" { + if strings.HasSuffix(existingFile, ".yaml") { + continue // Skip .yml if .yaml exists + }Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
809-809: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
| info.BasePathFromArg = "invalid/path" | ||
| expectedPath, _ = filepath.Abs("invalid/path") | ||
| result, err = configLoader.BasePathComputing(info) | ||
| assert.Error(t, err) | ||
| assert.Equal(t, "", result) |
There was a problem hiding this comment.
Remove ineffectual assignment.
The expectedPath variable is assigned but never used in the error case.
Apply this diff to fix the issue:
- expectedPath, _ = filepath.Abs("invalid/path")
result, err = configLoader.BasePathComputing(info)
assert.Error(t, err)
assert.Equal(t, "", result)📝 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.
| info.BasePathFromArg = "invalid/path" | |
| expectedPath, _ = filepath.Abs("invalid/path") | |
| result, err = configLoader.BasePathComputing(info) | |
| assert.Error(t, err) | |
| assert.Equal(t, "", result) | |
| info.BasePathFromArg = "invalid/path" | |
| result, err = configLoader.BasePathComputing(info) | |
| assert.Error(t, err) | |
| assert.Equal(t, "", result) |
🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: ineffectual assignment to expectedPath
(ineffassign)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/config/con.go (4)
25-25: Consider renaming constant for consistency with Go naming conventions.The constant
MaximumImportLvLshould be renamed tomaximumImportLevelormaxImportLevelfor better readability and consistency with Go naming conventions.-const MaximumImportLvL = 10 +const maxImportLevel = 10🧰 Tools
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
27-37: Add documentation for struct types.Consider adding documentation comments for both structs to explain their purpose and fields. This will improve code maintainability and help other developers understand the code better.
+// Imports represents a configuration import with its path and nesting level type Imports struct { Path string Level int } +// ConfigLoader handles the loading and management of Atmos configuration +// from various sources including files, environment variables, and defaults type ConfigLoader struct { viper *viper.Viper atmosConfig schema.AtmosConfiguration configFound bool debug bool AtmosConfigPaths []string }🧰 Tools
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
950-966: Remove or utilize unused function findGitTopLevel.The
findGitTopLevelfunction is well-implemented but currently unused. Consider either removing it or replacing the similar functionality ingetGitRepoRootwith this implementation.🧰 Tools
🪛 golangci-lint (1.62.2)
950-950: func
findGitTopLevelis unused(unused)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
918-918: Simplify string formatting.The argument is already a string, making
fmt.Sprintfunnecessary here.- u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) + u.LogWarning(atmosConfig, "error closing file '"+path+"'. "+err.Error())🧰 Tools
🪛 golangci-lint (1.62.2)
918-918: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/con.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/con.go
576-576: Error return value of cl.deepMergeConfig is not checked
(errcheck)
238-238: func (*ConfigLoader).loadSystemAndUserConfigs is unused
(unused)
950-950: func findGitTopLevel is unused
(unused)
918-918: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
740-740: printf: non-constant format string in call to fmt.Errorf
(govet)
853-853: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
🪛 GitHub Actions: Tests
pkg/config/con.go
[error] Invalid file path detected during git checkout process
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/config/con.go (4)
853-854:⚠️ Potential issueFix unused HasSuffix result in condition.
The result of
strings.HasSuffixis computed but not used in the condition, which might indicate a logical error.- } else if ext == ".yml" && strings.HasSuffix(existingFile, ".yaml") { + } else if ext == ".yml" && !strings.HasSuffix(existingFile, ".yaml") { continue // Keep .yaml priorityLikely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
853-853: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
576-576:⚠️ Potential issueHandle error returned by deepMergeConfig.
The error returned by
deepMergeConfigis not being checked, which could lead to silent failures.- cl.deepMergeConfig() + if err := cl.deepMergeConfig(); err != nil { + u.LogWarning(cl.atmosConfig, fmt.Sprintf("failed to merge configuration: %v", err)) + continue + }Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
576-576: Error return value of
cl.deepMergeConfigis not checked(errcheck)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
740-741:⚠️ Potential issueImprove error message formatting.
The error message uses string concatenation which could lead to formatting issues. Use
fmt.Errorfwith proper formatting verbs.- return fmt.Errorf(errorMessage) + return fmt.Errorf("no stack manifests found in the provided paths:\n%s\n\nCheck if `base_path`, 'stacks.base_path', 'stacks.included_paths' and 'stacks.excluded_paths' are correctly set in CLI config files or ENV vars", j)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
740-740: printf: non-constant format string in call to fmt.Errorf
(govet)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
238-238: Verify usage of loadSystemAndUserConfigs method.The
loadSystemAndUserConfigsmethod appears to contain important configuration loading logic but is currently unused. Either remove it if it's no longer needed or integrate it into the configuration loading flow.✅ Verification successful
Confirmed: loadSystemAndUserConfigs method is unused
The method is indeed unused in the codebase. No calls to this method were found, and it's not required by any interfaces. Consider either integrating it into the configuration loading flow or removing it to maintain code cleanliness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of loadSystemAndUserConfigs rg -l "loadSystemAndUserConfigs" --type goLength of output: 60
Script:
#!/bin/bash # Search for method calls and context echo "=== Method calls ===" rg "loadSystemAndUserConfigs" -B 2 -A 2 --type go echo -e "\n=== Potential interface definitions ===" ast-grep --pattern 'type $_ interface { $$$ loadSystemAndUserConfigs($$$) $$$ }'Length of output: 640
🧰 Tools
🪛 golangci-lint (1.62.2)
238-238: func
(*ConfigLoader).loadSystemAndUserConfigsis unused(unused)
🪛 GitHub Actions: Tests
[error] Invalid file path detected during git checkout process
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
pkg/config/loader.go (2)
500-500: Unused functionloadConfigsFromPath.The static analysis shows this function is unused. Removing or repurposing it may reduce code bloat.
- func (cl *ConfigLoader) loadConfigsFromPath(path string) (bool, []string, error) { - // ... - }🧰 Tools
🪛 golangci-lint (1.62.2)
500-500: func
(*ConfigLoader).loadConfigsFromPathis unused(unused)
876-876: Avoidfmt.Sprintfwith a plain string.Since the second argument is already a string, remove the
fmt.Sprintfcall to simplify.- u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) + u.LogWarning(atmosConfig, "error closing file '"+path+"'. "+err.Error())🧰 Tools
🪛 golangci-lint (1.62.2)
876-876: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
pkg/config/loader_test.go (1)
214-214: Check the write error.Static analysis suggests handling
w.Write's return value, in case of I/O errors.- w.Write([]byte("mock content")) + if _, err := w.Write([]byte("mock content")); err != nil { + t.Logf("Warning: write error: %v", err) + }🧰 Tools
🪛 golangci-lint (1.62.2)
214-214: Error return value of
w.Writeis not checked(errcheck)
pkg/config/config.go (3)
127-127: Fallback to defaults notice.Falling back to a default config is good for user experience, but consider logging a warning so users know a custom config wasn’t loaded.
132-132: Improve error message.Use a bit more context regarding the base path or environment to help debug path resolution issues.
- return atmosConfig, fmt.Errorf("failed to compute base path: %v", err) + return atmosConfig, fmt.Errorf("failed to compute base path '%s': %v", configAndStacksInfo.AtmosBasePath, err)
Line range hint
257-280: InprocessConfigFile, unify file checks.Merging config files works well, but you may unify error handling to avoid branching for
!fileExistsvsread errors. Reducing branching can enhance readability.pkg/config/utils.go (1)
799-810: Consider optimizing directory traversal.The current implementation could be more efficient:
- err := filepath.Walk(absPath, func(path string, info os.FileInfo, err error) error { + err := filepath.WalkDir(absPath, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } - if !info.IsDir() && (filepath.Ext(path) == ".yaml" || filepath.Ext(path) == ".yml") { + if !d.IsDir() && (filepath.Ext(path) == ".yaml" || filepath.Ext(path) == ".yml") {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/config/config.go(4 hunks)pkg/config/loader.go(1 hunks)pkg/config/loader_test.go(1 hunks)pkg/config/utils.go(2 hunks)
🧰 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_test.go
214-214: Error return value of w.Write is not checked
(errcheck)
pkg/config/loader.go
916-916: Error return value of cl.deepMergeConfig is not checked
(errcheck)
500-500: func (*ConfigLoader).loadConfigsFromPath is unused
(unused)
641-641: S1023: redundant return statement
(gosimple)
876-876: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
811-811: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/config/loader.go (3)
1-922: Well-structured new file.This file cleanly encapsulates Atmos' configuration loading workflow, allowing multiple config paths, environment variables, system paths, and merges. It’s an essential improvement over scattered logic. Good job!
🧰 Tools
🪛 golangci-lint (1.62.2)
916-916: Error return value of
cl.deepMergeConfigis not checked(errcheck)
500-500: func
(*ConfigLoader).loadConfigsFromPathis unused(unused)
641-641: S1023: redundant
returnstatement(gosimple)
876-876: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
811-811: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
641-641: Check for redundant return.The linter flagged a redundant
returnstatement. Verify if there’s an extrareturnor unneeded code.🧰 Tools
🪛 golangci-lint (1.62.2)
641-641: S1023: redundant
returnstatement(gosimple)
811-811: Validate usage ofstrings.HasSuffix.The static analysis warns that you might be ignoring the return value, but here it’s used in the condition. This could be a false positive. Double-check to confirm correctness.
🧰 Tools
🪛 golangci-lint (1.62.2)
811-811: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
pkg/config/loader_test.go (1)
1-276: Extensive test coverage.These tests cover multiple scenarios, from local to remote, ensuring robust merging. Great job!
🧰 Tools
🪛 golangci-lint (1.62.2)
214-214: Error return value of
w.Writeis not checked(errcheck)
pkg/config/config.go (2)
112-112: Loading logic is streamlined.Using
NewConfigLoaderhere neatly centralizes the configuration approach. Nicely done!
129-129: Store concatenated CLI config path.Storing the combined paths in
CliConfigPathis straightforward, ensuring they’re trackable.pkg/config/utils.go (3)
16-16: LGTM! Good choice of configuration management library.The addition of Viper is appropriate for robust configuration handling.
826-837: LGTM! Clean and thorough implementation.The function follows Go best practices with proper error handling and validation.
781-824: Consider adding security checks for file operations.The function handles configuration loading well, but could benefit from additional security measures:
- Validate file permissions to ensure configurations aren't world-readable
- Handle symlinks explicitly to prevent symlink attacks
- Implement path sanitization if
configPathis user-providedLet's verify the current file permissions handling:
| if found { | ||
| u.LogTrace(cl.atmosConfig, fmt.Sprintf("merge import paths: %v", resolvedPaths)) | ||
| } | ||
| cl.deepMergeConfig() |
There was a problem hiding this comment.
Check the error return.
cl.deepMergeConfig() returns an error, but it’s unhandled here, risking silent failures. Handle or log this error.
- cl.deepMergeConfig()
+ if err := cl.deepMergeConfig(); err != nil {
+ cl.debugLogging(fmt.Sprintf("error merge config after import: %s", err.Error()))
+ }📝 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.
| cl.deepMergeConfig() | |
| if err := cl.deepMergeConfig(); err != nil { | |
| cl.debugLogging(fmt.Sprintf("error merge config after import: %s", err.Error())) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
916-916: Error return value of cl.deepMergeConfig is not checked
(errcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/config/base_path.go (1)
123-124:⚠️ Potential issueFix incorrect variable in logging statement.
The logging statement uses
filePathwhich is undefined in this scope. It should usedirAbsinstead.- cl.debugLogging(fmt.Sprintf("base path from infra %s: %s", "git root", filePath)) + cl.debugLogging(fmt.Sprintf("base path from infra %s: %s", "git root", dirAbs))pkg/config/loader.go (1)
918-918:⚠️ Potential issueHandle error from deepMergeConfig.
The error returned by
cl.deepMergeConfig()should be handled.-cl.deepMergeConfig() +if err := cl.deepMergeConfig(); err != nil { + cl.debugLogging(fmt.Sprintf("error merging config after import: %s", err.Error())) +}🧰 Tools
🪛 golangci-lint (1.62.2)
918-918: Error return value of
cl.deepMergeConfigis not checked(errcheck)
🧹 Nitpick comments (9)
pkg/config/base_path.go (2)
25-26: Remove duplicate comment line.The comment "Check base path from configuration" appears twice consecutively.
// Check base path from configuration - // Check base path from configuration if cl.atmosConfig.BasePath != "" {
13-72: Consider adding error context for better debugging.The function handles multiple sources for base path resolution, but some error messages could be more descriptive.
- return "", err + return "", fmt.Errorf("failed to get current working directory: %w", err)pkg/config/loader.go (6)
23-23: Follow Go naming conventions for constants.The constant
MaximumImportLvLshould be renamed tomaximumImportLevelormaxImportLevelto follow Go naming conventions.-const MaximumImportLvL = 10 +const maxImportLevel = 10
50-57: Simplify debug logging conditions.The debug flag setting logic is duplicated and can be simplified.
-if logsLevelEnvVar == u.LogLevelDebug || logsLevelEnvVar == u.LogLevelTrace || configAndStacksInfo.LogsLevel == u.LogLevelDebug { - cl.debug = true - cl.LogsLevel = configAndStacksInfo.LogsLevel -} -if configAndStacksInfo.LogsLevel == u.LogLevelDebug || configAndStacksInfo.LogsLevel == u.LogLevelTrace { - cl.debug = true - cl.LogsLevel = configAndStacksInfo.LogsLevel -} +cl.debug = logsLevelEnvVar == u.LogLevelDebug || logsLevelEnvVar == u.LogLevelTrace || + configAndStacksInfo.LogsLevel == u.LogLevelDebug || configAndStacksInfo.LogsLevel == u.LogLevelTrace +if cl.debug { + cl.LogsLevel = configAndStacksInfo.LogsLevel +}
497-557: Remove unused function.The function
loadConfigsFromPathis not used anywhere in the codebase.Consider removing this unused function or document why it's being kept for future use.
🧰 Tools
🪛 golangci-lint (1.62.2)
497-497: func
(*ConfigLoader).loadConfigsFromPathis unused(unused)
637-639: Remove redundant return statement.The return statement at the end of
applyUserPreferencesis redundant.cl.debugLogging("No configuration found in user preferences") -return🧰 Tools
🪛 golangci-lint (1.62.2)
638-638: S1023: redundant
returnstatement(gosimple)
878-878: Simplify string formatting.Unnecessary use of fmt.Sprintf for a string that's already properly formatted.
-u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) +u.LogWarning(atmosConfig, "error closing file '"+path+"'. "+err.Error())🧰 Tools
🪛 golangci-lint (1.62.2)
878-878: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
46-46: Add comprehensive documentation for the configuration loading process.While the code is well-structured, it would benefit from more detailed documentation explaining:
- The configuration loading flowchart mentioned in the comment
- The priority order of different configuration sources
- The behavior of configuration merging
Consider adding a detailed comment block explaining these aspects to help future maintainers understand the system better.
pkg/config/base_path_test.go (1)
118-134: Improve test reliability and fix typo.
- Fix the comment typo:
- // test base path form "!repo-root" + // test base path from "!repo-root"
- Consider making the repo root path determination more robust:
- expectedPath, _ := filepath.Abs("../../") + expectedPath, _ := filepath.Abs(findRepoRoot(t)) + func findRepoRoot(t *testing.T) string { + t.Helper() + // Walk up the directory tree until we find .git + dir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get working directory: %v", err) + } + for { + if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("Could not find repository root") + } + dir = parent + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/exec/utils.go(1 hunks)pkg/config/base_path.go(1 hunks)pkg/config/base_path_test.go(1 hunks)pkg/config/const.go(1 hunks)pkg/config/loader.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/utils.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/loader.go
918-918: Error return value of cl.deepMergeConfig is not checked
(errcheck)
497-497: func (*ConfigLoader).loadConfigsFromPath is unused
(unused)
638-638: S1023: redundant return statement
(gosimple)
878-878: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
813-813: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
pkg/config/base_path_test.go
28-28: ineffectual assignment to expectedPath
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
pkg/config/const.go (1)
84-84: LGTM! Consider documenting the new YAML function.The new
AtmosGitRootFuncconstant follows the established naming patterns and integrates well with existing YAML functions. This addition will be valuable for resolving paths relative to the Git repository root, supporting the new atmos.d directory convention.Since documentation was emphasized in the PR comments, please ensure this new YAML function is documented in the Atmos CLI Configuration schema at https://atmos.tools/cli/configuration/. Would you like me to help draft the documentation?
pkg/config/base_path.go (3)
1-11: LGTM! Clean and well-organized imports.The imports are properly organized and aliased where necessary.
74-95: LGTM! Robust path validation implementation.The function thoroughly validates paths and provides clear error messages.
132-152: LGTM! Well-implemented Git root resolution.The function provides robust Git repository root detection with comprehensive error handling.
pkg/config/base_path_test.go (5)
1-11: LGTM! Clean and well-organized imports.The package declaration and imports are properly structured.
13-48: LGTM! Well-structured test with good coverage.The test effectively covers valid path, invalid path, and non-directory scenarios.
🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: ineffectual assignment to expectedPath
(ineffassign)
27-31: Remove ineffectual assignment.The
expectedPathvariable is assigned but never used in the error case.Apply this diff to fix the issue:
- expectedPath, _ = filepath.Abs("invalid/path") result, err = configLoader.BasePathComputing(info) assert.Error(t, err) assert.Equal(t, "", result)🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: ineffectual assignment to expectedPath
(ineffassign)
50-68: LGTM! Clean environment variable test.Good practices with proper setup and cleanup of both temporary directory and environment variable.
70-116: Improve error handling and test practices.While the test is comprehensive, there are several areas for improvement.
- Fix the comment formatting and typo:
- // test base_path Set in Configuration + // test base_path set in configuration
- Replace
os.Exit(1)witht.Fatal:- fmt.Printf("Failed to get the current working directory: %v\n", err) - os.Exit(1) // Exit with a non-zero code to indicate failure + t.Fatalf("Failed to get the current working directory: %v", err)
- Consider using helper functions to reduce the repetitive error handling code and improve readability:
func createTestDir(t *testing.T, parent, name string) string { t.Helper() path := filepath.Join(parent, name) if err := os.Mkdir(path, 0755); err != nil { t.Fatalf("Failed to create directory %q: %v", path, err) } return path }
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/config/loader.go (1)
918-918:⚠️ Potential issueHandle the error return from deepMergeConfig.
The error returned by
deepMergeConfig()should be handled to prevent silent failures.-cl.deepMergeConfig() +if err := cl.deepMergeConfig(); err != nil { + cl.debugLogging(fmt.Sprintf("error merging config after import: %s", err.Error())) +}🧰 Tools
🪛 golangci-lint (1.62.2)
918-918: Error return value of
cl.deepMergeConfigis not checked(errcheck)
🧹 Nitpick comments (6)
pkg/config/loader.go (6)
23-23: Add documentation for the MaximumImportLvL constant.Consider adding a documentation comment explaining the purpose and implications of this constant.
+// MaximumImportLvL defines the maximum depth of nested configuration imports +// to prevent infinite recursion in import chains. const MaximumImportLvL = 10
497-557: Remove or utilize the unused loadConfigsFromPath function.The
loadConfigsFromPathfunction is not being used anywhere in the codebase. Consider either removing it or utilizing it where appropriate.🧰 Tools
🪛 golangci-lint (1.62.2)
497-497: func
(*ConfigLoader).loadConfigsFromPathis unused(unused)
638-638: Remove redundant return statement.The return statement at the end of the function is redundant.
- return🧰 Tools
🪛 golangci-lint (1.62.2)
638-638: S1023: redundant
returnstatement(gosimple)
878-878: Simplify string concatenation.No need to use fmt.Sprintf when the argument is already a string.
-u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) +u.LogWarning(atmosConfig, "error closing file '"+path+"'. "+err.Error())🧰 Tools
🪛 golangci-lint (1.62.2)
878-878: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
813-813: Check HasSuffix return value.The HasSuffix check should be used in the condition.
-if ext == ".yaml" { +if ext == ".yaml" || (ext == ".yml" && !strings.HasSuffix(existingFile, ".yaml")) { priorityMap[key] = file -} else if ext == ".yml" && strings.HasSuffix(existingFile, ".yaml") { - continue }🧰 Tools
🪛 golangci-lint (1.62.2)
813-813: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
46-107: Consider improving testability of the configuration loading process.The
LoadConfigmethod handles multiple responsibilities and could benefit from:
- Breaking down the configuration loading steps into smaller, more testable functions
- Introducing interfaces for file system operations to facilitate mocking in tests
- Adding more structured error types for better error handling
This would make the code more maintainable and easier to test.
📜 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
918-918: Error return value of cl.deepMergeConfig is not checked
(errcheck)
497-497: func (*ConfigLoader).loadConfigsFromPath is unused
(unused)
638-638: S1023: redundant return statement
(gosimple)
878-878: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
813-813: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/config/loader.go (1)
1-21: Clean and well-organized imports!The package structure and imports are well-organized and all dependencies are properly declared.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/config/loader.go (1)
917-917:⚠️ Potential issueHandle the error return from deepMergeConfig.
The error from
deepMergeConfig()should be handled to ensure configuration integrity.Apply this fix:
-cl.deepMergeConfig() +if err := cl.deepMergeConfig(); err != nil { + cl.debugLogging(fmt.Sprintf("error merging config after import: %s", err.Error())) +}🧰 Tools
🪛 golangci-lint (1.62.2)
917-917: Error return value of
cl.deepMergeConfigis not checked(errcheck)
🧹 Nitpick comments (5)
pkg/config/loader.go (5)
23-23: Add documentation for the MaximumImportLvL constant.Consider adding a comment explaining the purpose and significance of this constant. This helps other developers understand why the value was chosen and what it represents.
+// MaximumImportLvL defines the maximum depth of nested configuration imports +// to prevent infinite recursion in circular dependencies const MaximumImportLvL = 10
497-557: Remove unused function loadConfigsFromPath.This function is not used anywhere in the codebase. Consider removing it or documenting why it's kept for future use.
🧰 Tools
🪛 golangci-lint (1.62.2)
497-497: func
(*ConfigLoader).loadConfigsFromPathis unused(unused)
637-637: Remove redundant return statement.The return statement here is redundant as the function would return implicitly.
cl.debugLogging("No configuration found in user preferences") -return🧰 Tools
🪛 golangci-lint (1.62.2)
637-637: S1023: redundant
returnstatement(gosimple)
877-877: Simplify string formatting.The argument is already a string, no need to use fmt.Sprintf here.
-u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) +u.LogWarning(atmosConfig, "error closing file '"+path+"'. "+err.Error())🧰 Tools
🪛 golangci-lint (1.62.2)
877-877: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
46-107: Consider implementing a more robust error handling strategy.The configuration loading process is critical for the application. Consider implementing a more comprehensive error handling strategy that:
- Distinguishes between fatal and non-fatal errors
- Provides detailed error context
- Maintains consistent error logging patterns
This would help in troubleshooting configuration issues more effectively.
📜 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
917-917: Error return value of cl.deepMergeConfig is not checked
(errcheck)
497-497: func (*ConfigLoader).loadConfigsFromPath is unused
(unused)
637-637: S1023: redundant return statement
(gosimple)
877-877: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
812-812: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
| if err != nil { | ||
| cl.debugLogging(fmt.Sprintf("system config file merged: %s", configSysPath)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Fix incorrect error handling logic.
The error handling is incorrect here. The code logs a success message when an error occurs.
Apply this fix:
if err != nil {
- cl.debugLogging(fmt.Sprintf("system config file merged: %s", configSysPath))
+ cl.debugLogging(fmt.Sprintf("error processing system config file: %s, error: %v", configSysPath, err))
continue
}📝 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 err != nil { | |
| cl.debugLogging(fmt.Sprintf("system config file merged: %s", configSysPath)) | |
| continue | |
| } | |
| if err != nil { | |
| cl.debugLogging(fmt.Sprintf("error processing system config file: %s, error: %v", configSysPath, err)) | |
| continue | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/config/loader.go (2)
925-925:⚠️ Potential issueHandle the error return from deepMergeConfig.
The error from
deepMergeConfig()should be checked to prevent silent failures.Apply this fix:
-cl.deepMergeConfig() +if err := cl.deepMergeConfig(); err != nil { + cl.debugLogging(fmt.Sprintf("error merge config after import: %s", err.Error())) +}🧰 Tools
🪛 golangci-lint (1.62.2)
925-925: Error return value of
cl.deepMergeConfigis not checked(errcheck)
654-657:⚠️ Potential issueFix incorrect error handling logic.
The error handling is incorrect here. The code logs a success message when an error occurs.
Apply this fix:
if err != nil { - cl.debugLogging(fmt.Sprintf("system config file merged: %s", configSysPath)) + cl.debugLogging(fmt.Sprintf("error processing system config file: %s, error: %v", configSysPath, err)) continue }
🧹 Nitpick comments (2)
pkg/config/loader.go (2)
23-41: Add documentation for exported types and members.Consider adding GoDoc comments for the exported types and members to improve code documentation:
+// MaximumImportLvL defines the maximum allowed depth for configuration imports const MaximumImportLvL = 10 +// Imports represents a configuration import with its path and nesting level type Imports struct { Path string Level int } +// ConfigLoader handles the loading and merging of Atmos configurations from various sources type ConfigLoader struct { viper *viper.Viper atmosConfig schema.AtmosConfiguration debug bool AtmosConfigPaths []string LogsLevel string } +// NewConfigLoader creates a new ConfigLoader instance with initialized viper func NewConfigLoader() *ConfigLoader {
505-505: Clean up code quality issues.Several code quality improvements can be made:
- The
loadConfigsFromPathfunction is unused and should be removed if not needed.- There's a redundant return statement in
applyUserPreferences.- Unnecessary use of fmt.Sprintf when the argument is already a string.
Apply these fixes:
-func (cl *ConfigLoader) loadConfigsFromPath(path string) (bool, []string, error) { - // ... remove entire unused function -} func (cl *ConfigLoader) applyUserPreferences() { // ... - return } if err != nil { - u.LogWarning(atmosConfig, fmt.Sprintf("%s", "error closing file '"+path+"'. "+err.Error())) + u.LogWarning(atmosConfig, "error closing file '"+path+"'. "+err.Error()) }Also applies to: 645-645, 885-885
🧰 Tools
🪛 golangci-lint (1.62.2)
505-505: func
(*ConfigLoader).loadConfigsFromPathis unused(unused)
📜 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
925-925: Error return value of cl.deepMergeConfig is not checked
(errcheck)
505-505: func (*ConfigLoader).loadConfigsFromPath is unused
(unused)
645-645: S1023: redundant return statement
(gosimple)
885-885: S1025: the argument is already a string, there's no need to use fmt.Sprintf
(gosimple)
820-820: SA4017: HasSuffix doesn't have side effects and its return value is ignored
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/config/loader.go (1)
1-21: Clean and well-organized imports!The imports are properly organized and all are being utilized effectively.
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/config/loader.go (2)
927-927:⚠️ Potential issueHandle the error return from deepMergeConfig.
The error from
cl.deepMergeConfig()is unhandled, which could lead to silent failures.Apply this fix:
- cl.deepMergeConfig() + if err := cl.deepMergeConfig(); err != nil { + cl.debugLogging(fmt.Sprintf("error merge config after import: %s", err.Error())) + }
656-659:⚠️ Potential issueFix incorrect error handling logic in loadSystemConfig.
The error handling is incorrect. The code logs a success message when an error occurs.
Apply this fix:
if err != nil { - cl.debugLogging(fmt.Sprintf("system config file merged: %s", configSysPath)) + cl.debugLogging(fmt.Sprintf("error processing system config file: %s, error: %v", configSysPath, err)) continue }
🧹 Nitpick comments (2)
pkg/config/loader.go (2)
25-25: Consider increasing the maximum import level.The current value of 10 for
MaximumImportLvLmight be too restrictive for complex configuration hierarchies. Consider increasing it to 20 or making it configurable.
421-427: Remove redundant logging statements.Multiple log messages about Git repository root not found are redundant and could cause confusion.
Apply this fix:
if err != nil { if err == os.ErrNotExist { cl.debugLogging(fmt.Sprintf("Git repository root not found: %s", gitRepoRoot)) } - cl.debugLogging(fmt.Sprintf("Git repository root not found: %s", gitRepoRoot)) } if isDir && err == nil { - cl.debugLogging(fmt.Sprintf("Git repository root not found: %s", gitRepoRoot)) + cl.debugLogging(fmt.Sprintf("Git repository root is valid: %s", gitRepoRoot)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/loader.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
| func (cl *ConfigLoader) debugLogging(msg string) { | ||
| if cl.debug { | ||
| cl.atmosConfig.Logs.Level = cl.LogsLevel | ||
| if cl.atmosConfig.Logs.Level == u.LogLevelTrace { | ||
| u.LogTrace(cl.atmosConfig, msg) | ||
| return | ||
| } | ||
| u.LogDebug(cl.atmosConfig, msg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add synchronization to prevent race conditions in debugLogging.
The debugLogging method modifies shared state (atmosConfig.Logs.Level) without synchronization, which could lead to race conditions in concurrent usage.
Consider using a mutex to protect access to shared state:
+type ConfigLoader struct {
+ mu sync.RWMutex
viper *viper.Viper
atmosConfig schema.AtmosConfiguration
debug bool
AtmosConfigPaths []string
LogsLevel string
}
func (cl *ConfigLoader) debugLogging(msg string) {
if cl.debug {
+ cl.mu.Lock()
cl.atmosConfig.Logs.Level = cl.LogsLevel
+ cl.mu.Unlock()
if cl.atmosConfig.Logs.Level == u.LogLevelTrace {
u.LogTrace(cl.atmosConfig, msg)
return
}
u.LogDebug(cl.atmosConfig, msg)
}
}Committable suggestion skipped: line range outside the PR's diff.
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
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
Load 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;3 Stages
--- config: layout: fixed --- flowchart TD A["Start Configuration Process"] --> Z1["Load Atmos Schema Defaults"] Z1 --> Z2["Load // go:embed cloudposse/atmos/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["Update ATMOS_CLI_CONFIG_PATH with loaded config absolute paths (separated by delimiter)"] C2 --> 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 --> 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 --> 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 --> 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
Based on the comprehensive summary of changes, here are the release notes:
Release Notes
New Features
Improvements
Bug Fixes
Documentation