Enhance Atmos Configuration with atmos.d Support and Import Functionality#1085
Enhance Atmos Configuration with atmos.d Support and Import Functionality#1085
Conversation
📝 WalkthroughWalkthroughThis pull request introduces extensive changes to the Atmos configuration system. It refactors the configuration loading functions and import handling logic within the package, updates the schema structure to include an import field, and adds new tests and documentation. Additionally, new YAML configuration files and enhanced logging are provided to support CLI configuration imports. Linter configurations have also been updated to recognize the term "import" where appropriate. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Definitions (1)pkg/config/config_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (9)
✨ 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:Details
❓ 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: 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:Details
✅ 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:Details
✅ 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/config/config_test.go (2)
157-185: Add test cases for error handling with importsThe current tests validate successful import scenarios, but there are no tests for error cases such as invalid import paths or malformed imported configurations.
Consider adding test cases for error handling scenarios like:
- Non-existent import path
- Invalid YAML in imported file
- Circular imports
218-219: Add error message to WriteFileThe error message from os.WriteFile could be enhanced to include the filename being written.
- err := os.WriteFile(path, []byte(content), 0o644) - require.NoError(t, err, "Failed to create config file") + err := os.WriteFile(path, []byte(content), 0o644) + require.NoError(t, err, "Failed to create config file %s", path)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/config_test.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
🧰 Additional context used
🧬 Code Definitions (1)
pkg/config/config_test.go (1)
pkg/config/config.go (1) (1)
InitCliConfig(25-62)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (12)
pkg/config/config_test.go (12)
10-10: Good addition of the require packageAdding the require package from testify is a good practice. It provides better error messages and stops test execution on failure, making tests more reliable and easier to debug.
16-37: Well-structured configuration setupMoving the configuration content to a variable at the top of the test function improves readability and maintainability. The YAML structure is clear and includes all necessary configuration sections.
38-46: Good use of test structure with testCaseThe testCase struct provides a clean way to organize parameterized tests. This approach makes it easier to add new test cases and maintain existing ones.
48-72: YAML extension test looks goodThe test case for the YAML extension is well-structured with clear setup and assertions. It properly validates all the important configuration properties.
73-92: YML extension test adds good coverageTesting both YAML and YML extensions is thorough and ensures the system works with both common extension formats.
93-110: Good fallback testThe test for invalid config file names properly verifies that the system falls back to default configuration values when an invalid file is provided.
120-120: Use filepath.Join consistentlyGood use of filepath.Join to create platform-independent paths. This ensures tests run correctly on all operating systems.
156-170: Good test for atmos.d supportThis test validates the new atmos.d convention support mentioned in the PR objectives. It confirms that configuration files in the .atmos.d directory are correctly loaded.
171-185: Good test for import functionalityThis test validates the new import functionality mentioned in the PR objectives. It confirms that configuration files can be imported from custom locations.
188-212: Well-structured test execution loopThe test execution loop is clean and handles all aspects of test setup and teardown. The use of t.Run for subtests provides better test output and organization.
215-220: Helpful utility functionThe createConfigFile function is a good abstraction that makes the test code cleaner and more maintainable.
222-233: Good directory managementThe changeWorkingDir function properly changes and restores the working directory, using t.Cleanup to ensure teardown happens even if tests fail.
aknysh
added
no-release
3 participants
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
./.Refactor
Documentation
Tests