Enhance Atmos Configuration with atmos.d Support and Import Functionality #1085
Conversation
📝 WalkthroughWalkthroughA new configuration management system has been implemented in the Atmos project, replacing legacy code with a simplified loading process. Key changes include the introduction of a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Atmos CLI
participant LC as LoadConfig
participant VC as Viper
participant IMS as Imports Processor
CLI->>LC: Invoke LoadConfig(configAndStacksInfo)
LC->>VC: Initialize Viper & set default configuration
LC->>VC: Load config from system, home, cwd, env, CLI
VC-->>LC: Return loaded configuration
LC->>IMS: Merge configuration imports recursively
IMS-->>LC: Return resolved configuration imports
LC->>CLI: Return merged AtmosConfiguration
sequenceDiagram
participant PI as processImports
participant R as Remote Import
participant L as Local Import
PI->>PI: Evaluate type of import path
alt Remote Import
PI->>R: processRemoteImport()
R-->>PI: Return remote configuration
else Local Import
PI->>L: processLocalImport()
L-->>PI: Return local configuration
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (14)
tests/fixtures/scenarios/atmos-cli-imports/configs.d/commands.yaml (2)
6-6: Remove Trailing Spaces in Command Step.
There are trailing spaces at the end of this line, which may lead to YAML lint warnings.Suggested diff:
- - atmos describe config + - atmos describe config🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: trailing spaces
(trailing-spaces)
7-7: Remove Excess Blank Line.
There is an extra blank line at the end of the file. Removing it will improve clarity.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 7-7: too many blank lines
(1 > 0) (empty-lines)
tests/fixtures/scenarios/atmos-configuration/atmos.d/commands.yaml (1)
3-4: Ensure Consistency in Command Descriptions.
Notice that the description here ("Run all tests with custom command") slightly differs from the one in the other configuration file ("Run all tests"). For a consistent CLI experience, consider aligning these descriptions if they are intended to perform the same action.pkg/config/load.go (4)
20-26: Clear documentation block.Your docstring is thorough. Consider adding a short usage snippet to guide new users.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 26-26: [golangci] pkg/config/load.go#L26
cyclomatic: function LoadConfig has cyclomatic complexity 13 (> max enabled 10) (revive)
210-210: Consolidate repeated string literals.You're logging
"error"and"path"multiple times across the codebase. Declaring them as constants could help reduce typos and improve consistency.- log.Debug("error process imports", "path", path, "error", err) + const ( + logKeyError = "error" + logKeyPath = "path" + ) + ... + log.Debug("error process imports", logKeyPath, path, logKeyError, err)
226-226: Use wrapped static errors to provide richer context.Instead of
fmt.Errorf("atmos config directory not found path %s", dirPath), consider:- return fmt.Errorf("atmos config directory not found path %s", dirPath) + return fmt.Errorf("atmos config directory not found path %s: %w", dirPath, ErrAtmosDirNotFound)🧰 Tools
🪛 GitHub Check: golangci
[failure] 226-226: [golangci] pkg/config/load.go#L226
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("atmos config directory not found path %s", dirPath)" (err113)
230-230: Avoid wildcard paths inside filepath.Join.Using
"atmos.d/**/*"withinfilepath.Joincan be brittle across platforms. You may directly concatenate or rely on glob utilities.🧰 Tools
🪛 GitHub Check: golangci
[failure] 230-230: [golangci] pkg/config/load.go#L230
filepathJoin: "atmos.d/**/*" contains a path separator (gocritic)pkg/config/imports.go (4)
36-36: Consider passing source by pointer.
source schema.AtmosConfigurationmay be large, so passing it as a pointer could reduce copying costs.🧰 Tools
🪛 GitHub Check: golangci
[failure] 36-36: [golangci] pkg/config/imports.go#L36
hugeParam: source is heavy (5776 bytes); consider passing it by pointer (gocritic)
68-68: Prefer wrapped static errors over dynamic ones.Replace these
fmt.Errorf(...)calls with well-defined error constants, wrapping them for context. This approach improves error handling consistency.Also applies to: 71-71, 74-74, 78-78, 117-117, 129-129, 149-149, 160-160, 174-174, 283-283, 373-373
🧰 Tools
🪛 GitHub Check: golangci
[failure] 68-68: [golangci] pkg/config/imports.go#L68
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("base_Path required to process imports")" (err113)
145-145: Simplify slice length check.Since
len(Imports)is zero for nil slices, you can omit the nil check and rely onlen(Imports) > 0.🧰 Tools
🪛 golangci-lint (1.62.2)
145-145: S1009: should omit nil check; len() for []string is defined as zero
(gosimple)
250-251: Review usage of "/.yaml" and "**/.yml" in filepath.Join.**Inline wildcards in
filepath.Joincan be error-prone. Using direct string concatenation or specialized file matching functions is often safer across environments.🧰 Tools
🪛 GitHub Check: golangci
[failure] 250-250: [golangci] pkg/config/imports.go#L250
filepathJoin: "**/*.yaml" contains a path separator (gocritic)
[failure] 251-251: [golangci] pkg/config/imports.go#L251
filepathJoin: "**/*.yml" contains a path separator (gocritic)pkg/config/import_test.go (1)
114-123: Consider refactoring HTTP handler to use switch statement.The current if-else chain could be more readable using a switch statement.
Apply this diff to improve readability:
- if r.URL.Path == "/config.yaml" { - fmt.Fprint(w, remoteContent) - } else if r.URL.Path == "/nested-remote.yaml" { - fmt.Fprint(w, nestedRemoteContent) - } else { - http.NotFound(w, r) - } + switch r.URL.Path { + case "/config.yaml": + fmt.Fprint(w, remoteContent) + case "/nested-remote.yaml": + fmt.Fprint(w, nestedRemoteContent) + default: + http.NotFound(w, r) + }🧰 Tools
🪛 GitHub Check: golangci
[failure] 115-115: [golangci] pkg/config/import_test.go#L115
ifElseChain: rewrite if-else to switch statement (gocritic)pkg/config/utils.go (1)
1-801: Consider splitting this file into smaller, focused files.The file is 667 lines long (exceeds 500-line limit). Consider breaking it down into smaller files based on functionality:
- Configuration file operations (search, merge)
- Environment variable processing
- Context and stack name handling
🧰 Tools
🪛 GitHub Check: golangci
[failure] 800-800: [golangci] pkg/config/utils.go#L800
file-length-limit: file length is 667 lines, which exceeds the limit of 500 (revive)tests/fixtures/scenarios/atmos-configuration/atmos.yaml (1)
1-6: Minor formatting adjustment needed.The configuration file is straightforward and meets its purpose of defining the default import path. However, static analysis has flagged trailing spaces on line 5. Please remove these trailing spaces to adhere to YAML best practices.
Proposed diff to remove trailing spaces on line 5:
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
pkg/config/README.md(1 hunks)pkg/config/config.go(1 hunks)pkg/config/default.go(1 hunks)pkg/config/import_test.go(1 hunks)pkg/config/imports.go(1 hunks)pkg/config/load.go(1 hunks)pkg/config/utils.go(3 hunks)pkg/schema/schema.go(1 hunks)tests/cli_test.go(1 hunks)tests/fixtures/scenarios/atmos-cli-imports/atmos.yaml(1 hunks)tests/fixtures/scenarios/atmos-cli-imports/configs.d/commands.yaml(1 hunks)tests/fixtures/scenarios/atmos-cli-imports/configs.d/tools/stack.yml(1 hunks)tests/fixtures/scenarios/atmos-cli-imports/configs.d/tools/terraform.yaml(1 hunks)tests/fixtures/scenarios/atmos-cli-imports/configs.d/vendor.yaml(1 hunks)tests/fixtures/scenarios/atmos-cli-imports/logs.yaml(1 hunks)tests/fixtures/scenarios/atmos-configuration/atmos.d/commands.yaml(1 hunks)tests/fixtures/scenarios/atmos-configuration/atmos.d/logs.yaml(1 hunks)tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/helmfile.yml(1 hunks)tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/stack.yaml(1 hunks)tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/terraform.yaml(1 hunks)tests/fixtures/scenarios/atmos-configuration/atmos.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)tests/test-cases/atmos-cli-imports.yaml(1 hunks)tests/test-cases/env.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/scenarios/atmos-configuration/atmos.d/logs.yaml
- tests/fixtures/scenarios/atmos-cli-imports/configs.d/vendor.yaml
- pkg/config/README.md
🧰 Additional context used
🧠 Learnings (2)
tests/fixtures/scenarios/atmos-cli-imports/atmos.yaml (1)
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos-cli-imports/atmos.yaml:8-8
Timestamp: 2025-01-25T15:21:40.413Z
Learning: In Atmos, when a directory is specified for configuration loading (e.g., in the `import` section of atmos.yaml), all files within that directory should be treated as Atmos configurations. Do not suggest restricting file extensions in directory-based glob patterns.
tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/helmfile.yml (1)
Learnt from: osterman
PR: cloudposse/atmos#808
File: examples/demo-atmos.d/atmos.d/tools/helmfile.yml:10-10
Timestamp: 2024-12-12T15:17:45.245Z
Learning: In `examples/demo-atmos.d/atmos.d/tools/helmfile.yml`, when suggesting changes to `kubeconfig_path`, ensure that the values use valid Go template syntax.
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/atmos-cli-imports/configs.d/commands.yaml
[error] 6-6: trailing spaces
(trailing-spaces)
[warning] 7-7: too many blank lines
(1 > 0) (empty-lines)
tests/fixtures/scenarios/atmos-configuration/atmos.yaml
[error] 5-5: trailing spaces
(trailing-spaces)
🪛 GitHub Check: golangci
pkg/config/utils.go
[failure] 800-800: [golangci] pkg/config/utils.go#L800
file-length-limit: file length is 667 lines, which exceeds the limit of 500 (revive)
pkg/config/import_test.go
[failure] 16-16: [golangci] pkg/config/import_test.go#L16
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 38-38: [golangci] pkg/config/import_test.go#L38
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 42-42: [golangci] pkg/config/import_test.go#L42
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 47-47: [golangci] pkg/config/import_test.go#L47
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 54-54: [golangci] pkg/config/import_test.go#L54
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 115-115: [golangci] pkg/config/import_test.go#L115
ifElseChain: rewrite if-else to switch statement (gocritic)
pkg/config/load.go
[failure] 242-242: [golangci] pkg/config/load.go#L242
add-constant: string literal "error" appears, at least, 4 times, create a named constant for it (revive)
[failure] 250-250: [golangci] pkg/config/load.go#L250
add-constant: string literal "path" appears, at least, 4 times, create a named constant for it (revive)
[failure] 26-26: [golangci] pkg/config/load.go#L26
cyclomatic: function LoadConfig has cyclomatic complexity 13 (> max enabled 10) (revive)
[failure] 226-226: [golangci] pkg/config/load.go#L226
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("atmos config directory not found path %s", dirPath)" (err113)
[failure] 230-230: [golangci] pkg/config/load.go#L230
filepathJoin: "atmos.d/**/*" contains a path separator (gocritic)
[failure] 239-239: [golangci] pkg/config/load.go#L239
filepathJoin: ".atmos.d/**/*" contains a path separator (gocritic)
pkg/config/imports.go
[failure] 128-128: [golangci] pkg/config/imports.go#L128
add-constant: string literal "error" appears, at least, 4 times, create a named constant for it (revive)
[failure] 148-148: [golangci] pkg/config/imports.go#L148
add-constant: string literal "import" appears, at least, 4 times, create a named constant for it (revive)
[failure] 173-173: [golangci] pkg/config/imports.go#L173
add-constant: string literal "path" appears, at least, 4 times, create a named constant for it (revive)
[failure] 68-68: [golangci] pkg/config/imports.go#L68
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("base_Path required to process imports")" (err113)
[failure] 71-71: [golangci] pkg/config/imports.go#L71
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("tempDir required to process imports")" (err113)
[failure] 74-74: [golangci] pkg/config/imports.go#L74
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("maximum import depth of %d exceeded", maxDepth)" (err113)
[failure] 78-78: [golangci] pkg/config/imports.go#L78
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("failed to resolve base path: %v", err)" (err113)
[failure] 117-117: [golangci] pkg/config/imports.go#L117
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("unsupported URL '%s': %v", importPath, err)" (err113)
[failure] 129-129: [golangci] pkg/config/imports.go#L129
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("failed to read remote config")" (err113)
[failure] 149-149: [golangci] pkg/config/imports.go#L149
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("failed to process nested imports")" (err113)
[failure] 160-160: [golangci] pkg/config/imports.go#L160
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("import_path required to process imports")" (err113)
[failure] 174-174: [golangci] pkg/config/imports.go#L174
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("failed to resolve local import path")" (err113)
[failure] 283-283: [golangci] pkg/config/imports.go#L283
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("no valid absolute paths found")" (err113)
[failure] 373-373: [golangci] pkg/config/imports.go#L373
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("no files matching patterns found")" (err113)
[failure] 250-250: [golangci] pkg/config/imports.go#L250
filepathJoin: "**/*.yaml" contains a path separator (gocritic)
[failure] 251-251: [golangci] pkg/config/imports.go#L251
filepathJoin: "**/*.yml" contains a path separator (gocritic)
[failure] 36-36: [golangci] pkg/config/imports.go#L36
hugeParam: source is heavy (5776 bytes); consider passing it by pointer (gocritic)
🪛 golangci-lint (1.62.2)
pkg/config/imports.go
145-145: S1009: should omit nil check; len() for []string is defined as zero
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (25)
tests/fixtures/scenarios/atmos-cli-imports/configs.d/commands.yaml (1)
1-7: Overall YAML structure looks good.
The file correctly defines the custom CLI command “test” with its description and steps.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: trailing spaces
(trailing-spaces)
[warning] 7-7: too many blank lines
(1 > 0) (empty-lines)
tests/fixtures/scenarios/atmos-configuration/atmos.d/commands.yaml (1)
1-7: YAML structure and content are set up correctly.
This file defines the “test” command with the expected key-value pairs.pkg/config/config.go (1)
18-18: Excellent simplification with new LoadConfig usage.This single call effectively removes extensive boilerplate, streamlining the flow.
tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden (1)
46-47: LGTM! The changes align with the PR objectives.The update to
cli_config_pathand addition of theimportarray prepare the configuration for the new import functionality.tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden (1)
59-60: LGTM! The configuration structure is comprehensive and well-organized.The configuration includes all necessary sections and maintains consistency with the import functionality.
pkg/config/default.go (2)
13-15: LGTM! The error message is user-friendly and actionable.The error message clearly explains the issue and provides a solution by referencing the sample configuration.
93-97:❓ Verification inconclusive
Verify the version check configuration values.
Please confirm if these default values are appropriate:
- Timeout: 1000 (units not specified)
- Frequency: "daily"
🌐 Web query:
What are the recommended timeout and frequency values for version checks in Go applications?💡 Result:
When implementing version checks in Go applications, recommended practices for timeouts and frequency depend on the specific use case and network conditions. Key considerations from industry practices:
Timeout Recommendations
HTTP-based version checks:
- Default timeout:
5 seconds[6]- Adaptive policy example:
policy := Adaptive( 200*time.Millisecond, // Initial attempt time.Second, // First retry after timeout 10*time.Second // Subsequent retries )- Maximum single attempt:
30 seconds(common web service limit)[7]Critical path operations:
- Database transactions:
15-30 seconds[4]- Script execution: Set via
RUNNER_SCRIPT_TIMEOUTwith value less than job timeout[1]Frequency Recommendations
Automated dependency checks:
- Daily scans for security updates[8]
- Align with release cycles (Go releases every 6 months)[5]
Application self-checks:
- Production: Every
5-15 minutesfor critical services[4]- Development/Staging: Every
24 hours[6]Implementation Patterns
// Example using context with timeout ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() req, _ := http.NewRequestWithContext(ctx, "GET", versionURL, nil) resp, err := http.DefaultClient.Do(req)Error Handling Considerations
- Use error wrapping to detect timeout causes:
if errors.Is(err, context.DeadlineExceeded) { // Handle timeout specifically }- Implement transaction rollback safeguards for timeout-induced state inconsistencies[4]
Testing Recommendations
- Validate with fault injection patterns:
// Test-specific slow response simulation if testingMode { time.Sleep(20 * time.Second) }- Verify timeout cascade effects through dependency chain[1][4]
These values should be adjusted based on observed latency percentiles and business requirements. For public APIs/CLIs checking upstream versions, a balance between freshness (15-30 minute intervals) and reliability (retry budgets) is typically optimal[6][7].
Citations:
- 1: https://docs.gitlab.com/ci/runners/configure_runners/
- 2: https://go.dev/ref/mod
- 3: https://adam-p.ca/blog/2022/01/golang-http-server-timeouts/
- 4: https://www.piiano.com/blog/golang-timeout
- 5: https://zchee.github.io/golang-wiki/Go-Release-Cycle/
- 6: https://pkg.go.dev/github.com/gogama/httpx/timeout
- 7: https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
- 8: https://docs.gitlab.com/ee/development/go_guide/go_upgrade.html
- 9: https://tip.golang.org/doc/gc-guide
- 10: x/time/rate: Wait can be slow to allow events golang/go#37058
Review Update: Confirm Default Timeout and Frequency Configuration
The default configuration in
pkg/config/default.go(lines 93‑97):Check: schema.VersionCheck{ Enabled: true, Timeout: 1000, Frequency: "daily", },has raised two points for verification:
Timeout:
Current value is set to1000. If this represents milliseconds, it implies a 1‑second timeout. However, best practices for HTTP-based version checks generally recommend a default timeout closer to 5 seconds (5000 ms). Please verify whether the unit is indeed milliseconds and, if so, consider if a higher default would yield more reliable checks.Frequency:
The default of"daily"is acceptable for automated dependency checks. Nonetheless, depending on the operational context (e.g., production vs. development), more frequent checks (such as every 5–15 minutes) might be advisable. Please assess if"daily"aligns with the intended use case.Thanks for reviewing these configuration values to ensure they meet the required operational criteria.
tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden (1)
1-18: LGTM! The debug logs demonstrate proper configuration loading behavior.The logs show:
- Expected handling of missing .atmos.d files
- Successful merging of configuration files
- Appropriate debug level information
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
148-149: LGTM! Configuration structure aligns with the new import functionality.The changes correctly reflect the new import capability by adding the
importfield and updating thecli_config_pathto explicitly reference the YAML file.tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
162-165:✅ Verification successful
Verify the remote import URL.
The import configuration includes both remote and local paths, which is good. However, let's verify the remote URL:
🏁 Script executed:
#!/bin/bash # Description: Verify the remote URL is accessible curl -I https://raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yamlLength of output: 1279
Remote URL Verified: Import Configuration is Good
The remote URL has been confirmed accessible (HTTP/2 200), and the import configuration correctly includes both remote and local paths. No further changes are needed.
pkg/config/import_test.go (2)
20-91: LGTM! Comprehensive test coverage for import processing.The test cases effectively cover various scenarios including:
- Remote URL imports
- Recursive directory imports
- Local file imports
- Invalid paths
- Error handling
🧰 Tools
🪛 GitHub Check: golangci
[failure] 38-38: [golangci] pkg/config/import_test.go#L38
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 42-42: [golangci] pkg/config/import_test.go#L42
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 47-47: [golangci] pkg/config/import_test.go#L47
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 54-54: [golangci] pkg/config/import_test.go#L54
G306: Expect WriteFile permissions to be 0600 or less (gosec)
93-161: LGTM! Well-structured nested import tests.The test cases thoroughly validate:
- Local nested imports
- Remote nested imports
- Depth limit handling
- Mixed import scenarios
🧰 Tools
🪛 GitHub Check: golangci
[failure] 115-115: [golangci] pkg/config/import_test.go#L115
ifElseChain: rewrite if-else to switch statement (gocritic)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden (1)
1-26: LGTM! Comprehensive debug logging for import processing.The debug logs effectively capture:
- Failed glob matches with clear error messages
- Successful configuration merges
- Processing of store configurations
pkg/config/utils.go (1)
785-800:✅ Verification successful
LGTM! Clean implementation of config file merging.
The function follows Go best practices with proper error handling for both file reading and config merging operations.
Let's verify the error handling by checking if the function is used correctly in the codebase:
🏁 Script executed:
#!/bin/bash # Search for MergeConfigFile usage to ensure errors are properly handled rg -A 3 "MergeConfigFile\("Length of output: 701
Excellent work with the MergeConfigFile function!
The implementation in
pkg/config/utils.goelegantly handles file reading and configuration merging with proper error propagation. Our verification of its usage in bothpkg/config/imports.goandpkg/config/load.goconfirms that errors are checked and logged, ensuring robust handling.🧰 Tools
🪛 GitHub Check: golangci
[failure] 800-800: [golangci] pkg/config/utils.go#L800
file-length-limit: file length is 667 lines, which exceeds the limit of 500 (revive)tests/cli_test.go (1)
314-317: LGTM! Good addition to test output sanitization.The new regex pattern effectively handles dynamic import file paths, ensuring consistent test output by standardizing paths containing random numbers.
pkg/schema/schema.go (1)
41-41: LGTM! Well-structured addition of Import field.The new field follows the struct's tag conventions and uses an appropriate type for storing multiple import paths.
tests/fixtures/scenarios/atmos-cli-imports/logs.yaml (1)
1-4: Verify if Debug level is intended for this configuration.While the configuration is well-structured, Debug level logging can be verbose and impact performance. Please confirm if this level is specifically needed for these test fixtures.
tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/stack.yaml (1)
1-8: Clear and Consistent "stacks" ConfigurationThis new YAML configuration clearly defines the stacks section with all essential keys:
- base_path is set appropriately,
- included_paths and excluded_paths use glob patterns effectively,
- name_pattern is defined to be "{dev}" for dynamic naming.
Everything is consistent and follows the intended structure for the PR’s configuration enhancements.
tests/fixtures/scenarios/atmos-cli-imports/configs.d/tools/stack.yml (1)
1-8: Mirrored "stacks" Configuration for ConsistencyThis file mirrors the configuration seen in
atmos-configuration/atmos.d/tools/stack.yaml. Reusing the same structure across different scenarios enhances consistency.tests/fixtures/scenarios/atmos-cli-imports/configs.d/tools/terraform.yaml (1)
1-8: Well-Defined Terraform Component SettingsThe new Terraform configuration section under components.terraform is clear:
- The base_path is set to "components/terraform",
- Boolean flags for auto-approval, initialization running, reconfiguration, and backend file generation are appropriately defined.
These settings provide a structured way to manage Terraform components as per the PR objectives.
tests/test-cases/env.yaml (1)
17-19: Updated Expected Regex for atmos_cli_config_pathThe expected output for
atmos_cli_config_pathnow correctly matches a pattern ending with "atmos.yaml". This change reflects the updated configuration filename requirements and aligns with the overall PR objectives for enhanced configuration clarity.tests/fixtures/scenarios/atmos-cli-imports/atmos.yaml (1)
1-10: Robust Import Configuration FileThis new
atmos.yamlfile demonstrates the new import functionality by:
- Including comments that concisely explain the purpose and behavior,
- Defining a base_path and an import list with diverse sources: remote URL, a glob pattern for directory-based imports, and a specific file.
Notably, the glob pattern ("configs.d/**/*") does not restrict file extensions, which aligns with the guidance from long-term learnings. The file is clear, well documented, and meets the PR objectives for import functionality.
tests/test-cases/atmos-cli-imports.yaml (1)
1-29: Clear and well-structured test cases for configuration imports.The test cases effectively validate the new import configuration functionality, ensuring that both import-specific and general configuration paths are exercised. The descriptions and snapshot settings are concise and informative.
tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/helmfile.yml (1)
1-15: Well-documented Helmfile configuration.The configuration section clearly explains each parameter with alternative methods for overriding via environment variables or command-line arguments. One point to verify: please ensure that the
kubeconfig_pathvalue ("/dev/shm") complies with valid Go template syntax if the implementation requires it.tests/fixtures/scenarios/atmos-configuration/atmos.d/tools/terraform.yaml (1)
1-24: Comprehensive Terraform configuration section.All configuration settings for Terraform, including defaults and optional overrides, are clearly laid out with supporting comments. Confirm that the environment variable names and default values match the conventions used elsewhere in the project.
|
💥 This pull request now has conflicts. Could you fix it @haitham911? 🙏 |
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/config/load.go (3)
19-19: Consider removing the unused constant.
MaximumImportLvLis declared but never referenced in this file. If it's not used elsewhere, removing it will help reduce clutter.
80-100: Optional: Merge repeated reading logic into a single helper.
loadConfigSourcescalls multiple small functions for each location. While it's clear, consider using a table-driven approach to reduce repetition.
95-95: Fix the function name to maintain naming consistency.
readEnvAmosConfigPathis likely intended to bereadEnvAtmosConfigPath. Update both definition and call sites:- if err := readEnvAmosConfigPath(v); err != nil { + if err := readEnvAtmosConfigPath(v); err != nil {- func readEnvAmosConfigPath(v *viper.Viper) error { + func readEnvAtmosConfigPath(v *viper.Viper) error {Also applies to: 164-164
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/load.go(1 hunks)pkg/schema/schema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schema/schema.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/config/load.go (4)
23-27: Documentation is concise and helpful.These lines provide a clear explanation of the config-loading order. Nicely done.
28-68: LoadConfig function appears logically organized.It sets up defaults, merges multiple sources, and applies fallback config if none is found. This is a straightforward approach.
219-222: Same approach to handling import errors as before.You’re still logging errors from the default imports and continuing. Refer to previous discussions if the logic should change or remain.
303-339: Kudos on splitting out processScalarNode.The function’s structure is cleaner now. Overall approach to recursive YAML processing is readable and modular.
| if err := yaml.Unmarshal(yamlContent, &rootNode); err != nil { | ||
| log.Debug("failed to parse YAML", "content", yamlContent, "error", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Avoid logging entire YAML content to prevent potential data leaks.
Printing the fully parsed config content, particularly at debug, might risk exposing sensitive values. Consider logging only the error or a sanitized version of the content.
- log.Debug("failed to parse YAML", "content", yamlContent, "error", err)
+ log.Debug("failed to parse YAML", "error", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := yaml.Unmarshal(yamlContent, &rootNode); err != nil { | |
| log.Debug("failed to parse YAML", "content", yamlContent, "error", err) | |
| return err | |
| } | |
| if err := yaml.Unmarshal(yamlContent, &rootNode); err != nil { | |
| log.Debug("failed to parse YAML", "error", err) | |
| return err | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
144-148: Consider replacing absolute paths.Using absolute paths in the configuration might reduce portability across environments. Relative or parameterized paths would be more flexible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
148-149: Proper implementation of the new import fieldThe addition of the "import" field with a null value looks correct and aligns well with the PR's objective to introduce import functionality within atmos.yaml configuration files.
tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
162-166: Verify remote configuration sources.For each remote import, ensure authenticity and adopt error handling for untrusted or unreachable URLs. This guards against configuration tampering.
| color: true | ||
| cli_config_path: /absolute/path/to/repo/tests/fixtures/scenarios/atmos-cli-imports | ||
| import: | ||
| - https:/raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yaml |
There was a problem hiding this comment.
Check URL formatting.
"https:/raw.githubusercontent.com" should likely be "https://raw.githubusercontent.com". The missing slash could cause import failures.
- - https:/raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yaml
+ - https://raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yaml📝 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.
| - https:/raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yaml | |
| - https://raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yaml |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1085 +/- ##
==========================================
+ Coverage 17.17% 18.18% +1.01%
==========================================
Files 169 173 +4
Lines 18725 19172 +447
==========================================
+ Hits 3216 3487 +271
- Misses 14924 15074 +150
- Partials 585 611 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/config.go (1)
108-108: Consider passingconfigAndStacksInfoby pointer.
Static analysis suggests thatconfigAndStacksInfois large (approximately 1088 bytes). Passing it by pointer inprocessStackConfigsmay help avoid unnecessary copying of large struct data.-func processStackConfigs(atmosConfig *schema.AtmosConfiguration, configAndStacksInfo schema.ConfigAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths []string) error { +func processStackConfigs(atmosConfig *schema.AtmosConfiguration, configAndStacksInfo *schema.ConfigAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths []string) error {🧰 Tools
🪛 GitHub Check: golangci
[failure] 108-108: [golangci] pkg/config/config.go#L108
hugeParam: configAndStacksInfo is heavy (1088 bytes); consider passing it by pointer (gocritic)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/config.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
pkg/config/config.go
[failure] 108-108: [golangci] pkg/config/config.go#L108
hugeParam: configAndStacksInfo is heavy (1088 bytes); consider passing it by pointer (gocritic)
[failure] 135-135: [golangci] pkg/config/config.go#L135
SA1019: u.LogTrace is deprecated: Use log.Debug instead. This function will be removed in a future release. LogTrace logs the provided trace message (staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/config/config.go (1)
18-18: The newLoadConfiginvocation looks clean and direct.
It's a straightforward call to load Atmos configuration and simplifies your initialization logic.
pkg/config/config.go
Outdated
| atmosConfig.StackConfigFilesRelativePaths = stackConfigFilesRelativePaths | ||
|
|
||
| if stackIsPhysicalPath { | ||
| u.LogTrace(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use log.Debug instead of the deprecated u.LogTrace.
u.LogTrace is marked for removal in a future release. Migrate to log.Debug to avoid potential breakage.
- u.LogTrace(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n",
+ log.Debug(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n",📝 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.
| u.LogTrace(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n", | |
| log.Debug(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n", |
🧰 Tools
🪛 GitHub Check: golangci
[failure] 135-135: [golangci] pkg/config/config.go#L135
SA1019: u.LogTrace is deprecated: Use log.Debug instead. This function will be removed in a future release. LogTrace logs the provided trace message (staticcheck)
pkg/config/config.go
Outdated
| if processStacks { | ||
| // If the specified stack name is a logical name, find all stack manifests in the provided paths | ||
| stackConfigFilesAbsolutePaths, stackConfigFilesRelativePaths, stackIsPhysicalPath, err := FindAllStackConfigsInPathsForStack( | ||
| atmosConfig, | ||
| configAndStacksInfo.Stack, | ||
| includeStackAbsPaths, | ||
| excludeStackAbsPaths, | ||
| ) | ||
| if err != nil { | ||
| return atmosConfig, err | ||
| } | ||
|
|
||
| if len(stackConfigFilesAbsolutePaths) < 1 { | ||
| j, err := u.ConvertToYAML(includeStackAbsPaths) | ||
| if processStacks { | ||
| err = processStackConfigs(&atmosConfig, configAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid the redundant if processStacks condition.
Having two consecutive if processStacks statements is confusing and unnecessary. It can be safely combined into a single conditional check.
Apply this diff to remove the duplicated condition:
- if processStacks {
- if processStacks {
- err = processStackConfigs(&atmosConfig, configAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths)
- if err != nil {
- return atmosConfig, err
- }
- }
- }
+ if processStacks {
+ err = processStackConfigs(&atmosConfig, configAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths)
+ if err != nil {
+ return atmosConfig, err
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if processStacks { | |
| // If the specified stack name is a logical name, find all stack manifests in the provided paths | |
| stackConfigFilesAbsolutePaths, stackConfigFilesRelativePaths, stackIsPhysicalPath, err := FindAllStackConfigsInPathsForStack( | |
| atmosConfig, | |
| configAndStacksInfo.Stack, | |
| includeStackAbsPaths, | |
| excludeStackAbsPaths, | |
| ) | |
| if err != nil { | |
| return atmosConfig, err | |
| } | |
| if len(stackConfigFilesAbsolutePaths) < 1 { | |
| j, err := u.ConvertToYAML(includeStackAbsPaths) | |
| if processStacks { | |
| err = processStackConfigs(&atmosConfig, configAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths) | |
| if processStacks { | |
| err = processStackConfigs(&atmosConfig, configAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths) | |
| if err != nil { | |
| return atmosConfig, err | |
| } | |
| } |
| log "github.com/charmbracelet/log" | ||
| "github.com/cloudposse/atmos/pkg/schema" | ||
| "github.com/cloudposse/atmos/pkg/version" | ||
| "github.com/mitchellh/go-homedir" |
| for _, directive := range allowedDirectives { | ||
| if node.Tag == directive { | ||
| arg := node.Value | ||
| if directive == AtmosYamlFuncEnv { |
There was a problem hiding this comment.
Seems like we are reimplementing this. What about the other yaml functions? This was only one example.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/config.go (1)
18-18: Consider passing large struct by pointer to improve performance.The
configAndStacksInfoparameter is 1088 bytes which makes it expensive to pass by value. While you're already using pointers in other functions, theInitCliConfigsignature still passes it by value.- func InitCliConfig(configAndStacksInfo schema.ConfigAndStacksInfo, processStacks bool) (schema.AtmosConfiguration, error) { + func InitCliConfig(configAndStacksInfo *schema.ConfigAndStacksInfo, processStacks bool) (schema.AtmosConfiguration, error) {Remember to update the function call on line 19 as well:
- atmosConfig, err := processAtmosConfigs(&configAndStacksInfo) + atmosConfig, err := processAtmosConfigs(configAndStacksInfo)🧰 Tools
🪛 GitHub Check: golangci
[failure] 18-18: [golangci] pkg/config/config.go#L18
hugeParam: configAndStacksInfo is heavy (1088 bytes); consider passing it by pointer (gocritic)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/config.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
pkg/config/config.go
[failure] 18-18: [golangci] pkg/config/config.go#L18
hugeParam: configAndStacksInfo is heavy (1088 bytes); consider passing it by pointer (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/config/config.go (6)
7-7: Good addition of the structured logger package.Adding the Charm logger package aligns with the code's modernization efforts and supports the improved logging mentioned in the summary.
18-32: Excellent refactoring of InitCliConfig for better separation of concerns.The function has been simplified by delegating configuration processing to
processAtmosConfigs. The addition of handling forAtmosBasePathand ensuring theAppendUserAgentis set with version information improves maintainability.🧰 Tools
🪛 GitHub Check: golangci
[failure] 18-18: [golangci] pkg/config/config.go#L18
hugeParam: configAndStacksInfo is heavy (1088 bytes); consider passing it by pointer (gocritic)
45-50: Good cleanup of processStacks conditional.This implementation resolves the previously reported issue of redundant nested
if processStacksconditions. Now there's a single clean conditional block that processes stack configurations when needed.
56-79: Great extraction of configuration loading logic.The new
processAtmosConfigsfunction nicely encapsulates the configuration loading workflow, starting with the newLoadConfigfunction and proceeding through environment variables, command-line arguments, and store configuration processing.
81-122: Good simplification of error handling in atmosConfigAbsolutePaths.The direct error returns without wrapping improve code readability while maintaining the functionality of converting paths to absolute paths.
124-161: Well-structured stack configuration processing.The new
processStackConfigsfunction properly handles finding stack configurations and sets the appropriate configuration properties. The use oflog.Debugat line 151 correctly implements the logging recommendation from previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/config/config.go (5)
19-19: Fix typo in TODO comment.There's a typo in your comment: "pinter" should be "pointer".
-//nolint:gocritic // TODO: Change configAndStacksInfo to pinter . Temporarily suppressing gocritic warnings; refactoring InitCliConfig would require extensive changes. +//nolint:gocritic // TODO: Change configAndStacksInfo to pointer . Temporarily suppressing gocritic warnings; refactoring InitCliConfig would require extensive changes.
19-20: Inconsistent pointer usage in code vs. comment.Your TODO comment suggests changing
configAndStacksInfoto a pointer, but you're already using pointers in function calls like in line 48. Consider removing or updating this TODO comment.-//nolint:gocritic // TODO: Change configAndStacksInfo to pinter . Temporarily suppressing gocritic warnings; refactoring InitCliConfig would require extensive changes. +//nolint:gocritic // Temporarily suppressing gocritic warnings; refactoring InitCliConfig would require extensive changes.
58-81: Add function documentation to processAtmosConfigs.This function would benefit from a comment explaining its purpose and how it relates to the new import functionality.
+// processAtmosConfigs loads the Atmos configuration using LoadConfig which supports +// the atmos.d convention and import functionality, then processes environment variables, +// command-line arguments, and store configurations. func processAtmosConfigs(configAndStacksInfo *schema.ConfigAndStacksInfo) (schema.AtmosConfiguration, error) {
126-163: Add function documentation to processStackConfigs.This function would benefit from a comment explaining its purpose in supporting the stack configuration processing.
+// processStackConfigs finds all stack configuration files in the provided paths for the specified stack, +// determines if the stack is a physical path or logical name, and updates the configuration accordingly. func processStackConfigs(atmosConfig *schema.AtmosConfiguration, configAndStacksInfo *schema.ConfigAndStacksInfo, includeStackAbsPaths, excludeStackAbsPaths []string) error {
153-155: Simplify log message formatting.The log message has unnecessary newlines at both the beginning and end.
-log.Debug(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n", +log.Debug(fmt.Sprintf("The stack '%s' matches the stack manifest %s", configAndStacksInfo.Stack, stackConfigFilesRelativePaths[0]))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/config.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/config/config.go (5)
21-21: The new configuration loading approach looks good.Nice refactoring to simplify configuration loading by extracting it to a dedicated function.
25-29: Clear handling of Terraform provider base path.Your comments and code are clear about the precedence of base path settings.
47-52: Clean stack configuration processing.Good extraction of stack configuration logic into a separate function to support the atmos.d convention.
153-155: Fixed deprecated logging method.Good job replacing the deprecated
u.LogTracewithlog.Debugas recommended.
59-59:❓ Verification inconclusive
Verify LoadConfig implementation.
The
LoadConfigfunction called here is where the main atmos.d support and import functionality is implemented. Make sure it properly handles all the scenarios described in the PR objectives.
🏁 Script executed:
#!/bin/bash # Find the LoadConfig function implementation to verify it supports atmos.d and imports rg -A 5 "func LoadConfig" pkg/config/Length of output: 400
Action: Verify that LoadConfig fully implements atmos.d support and import handling.
The initial part of LoadConfig in pkg/config/load.go shows that viper is properly configured and defaults are set. However, the code snippet only displays the setup portion (lines 1–5 of the function). Please ensure that the remaining implementation correctly processes atmos.d configuration directives and import functionality as outlined in the PR objectives.
- Confirm that subsequent logic in LoadConfig handles atmos.d support.
- Check any import resolution and error handling related to atmos.d usage.
| @@ -0,0 +1,166 @@ | |||
| base_path: "" | |||
There was a problem hiding this comment.
Shouldn't this have the derived base path?
| auto_generate_backend_file: false | ||
| command: terraform | ||
| shell: | ||
| prompt: "" |
There was a problem hiding this comment.
I think for other describe commands we have empty values not displayed by default and a flag that allows us to include empty.
| color: true | ||
| cli_config_path: /absolute/path/to/repo/tests/fixtures/scenarios/atmos-cli-imports | ||
| import: | ||
| - https:/raw.githubusercontent.com/cloudposse/atmos/refs/heads/main/atmos.yaml |
There was a problem hiding this comment.
Is this typo deliberate? If not deliberate then we have a problem with tests passing and not catching the invalid URL
| log.Debug(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n", | ||
| configAndStacksInfo.Stack, | ||
| stackConfigFilesRelativePaths[0])) | ||
| atmosConfig.StackType = "Directory" |
There was a problem hiding this comment.
Not sure if this makes sense to me. What is a "directory" stack type?
Cc @aknysh
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.why
references
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests