Fix ATMOS_CLI_CONFIG_PATH and ATMOS_BASE_PATH ha
8000
ndling
#1096
Conversation
ATMOS_CLI_CONFIG_PATH and ATMOS_BASE_PATH handling
📝 WalkthroughWalkthroughThis pull request standardizes how paths are handled across several modules. In both the Helmfile and Terraform executors, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Func as Execution Function
participant Abs as filepath.Abs
participant Env as Environment List
Caller->>Func: Invoke command execution
Func->>Abs: Compute absolute path (base path)
Abs-->>Func: Return absPath or error
alt Error encountered
Func-->>Caller: Return error immediately
else Successful calculation
Func->>Env: Append "ATMOS_BASE_PATH=absPath"
Func-->>Caller: Continue execution
end
Possibly related PRs
Suggested reviewers
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test-cases/demo-stacks.yaml (1)
153-157: Intentionally Disabling the 'atmos_vendor_pull' Test Case
The change setsenabled: falsefor theatmos_vendor_pulltest case. Please confirm that this decision is intentional and that you have documented the rationale elsewhere (e.g., in the PR description or issue comments). This test case will no longer run, so ensure that any critical validations provided by it are either no longer needed or have alternative coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test-cases/demo-stacks.yaml(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
tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/config/config_test.go (1)
11-12: Missing period in commentThe comment at line 11 should end with a period for consistency with Go's documentation style guidelines.
-// test base path and atmosConfigFilePath +// Test base path and atmosConfigFilePath.🧰 Tools
🪛 GitHub Check: golangci
[failure] 11-11: [golangci] pkg/config/config_test.go#L11
Comment should end in a period (godot)tests/cli_terraform_test.go (1)
220-227: Improve environment variable validationThe test only checks if the environment variable names appear in the output, but doesn't validate their values. Consider enhancing the test to also check that the values are correct (paths are absolute).
- // Check ATMOS_BASE_PATH and ATMOS_CLI_CONFIG_PATH exported on stdout - ATMOS_BASE_PATH := "ATMOS_BASE_PATH" - ATMOS_CLI_CONFIG_PATH := "ATMOS_CLI_CONFIG_PATH" - if !strings.Contains(stdout.String(), ATMOS_BASE_PATH) { - t.Errorf("Expected output not found in stdout: %s", ATMOS_BASE_PATH) - } - if !strings.Contains(stdout.String(), ATMOS_CLI_CONFIG_PATH) { - t.Errorf("Expected output not found in stdout: %s", ATMOS_CLI_CONFIG_PATH) - } + // Check ATMOS_BASE_PATH and ATMOS_CLI_CONFIG_PATH exported on stdout with correct values + basePathKey := "ATMOS_BASE_PATH=" + cliConfigPathKey := "ATMOS_CLI_CONFIG_PATH=" + + // Extract the values from the output + output := stdout.String() + if !strings.Contains(output, basePathKey) { + t.Errorf("Expected output not found in stdout: %s", basePathKey) + } else { + // Find the line with the base path + lines := strings.Split(output, "\n") + for _, line := range lines { + if strings.Contains(line, basePathKey) { + value := strings.TrimSpace(strings.Split(line, basePathKey)[1]) + // Check that it's an absolute path + if !filepath.IsAbs(value) { + t.Errorf("ATMOS_BASE_PATH is not an absolute path: %s", value) + } + break + } + } + } + + if !strings.Contains(output, cliConfigPathKey) { + t.Errorf("Expected output not found in stdout: %s", cliConfigPathKey) + } else { + // Find the line with the CLI config path + lines := strings.Split(output, "\n") + for _, line := range lines { + if strings.Contains(line, cliConfigPathKey) { + value := strings.TrimSpace(strings.Split(line, cliConfigPathKey)[1]) + // Check that it's an absolute path + if !filepath.IsAbs(value) { + t.Errorf("ATMOS_CLI_CONFIG_PATH is not an absolute path: %s", value) + } + break + } + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/config_test.go(1 hunks)tests/cli_terraform_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/config_test.go
28-28: Error return value of f.WriteString is not checked
(errcheck)
29-29: Error return value of f.WriteString is not checked
(errcheck)
30-30: Error return value of f.WriteString is not checked
(errcheck)
31-31: Error return value of f.WriteString is not checked
(errcheck)
38-38: Error return value of os.Chdir is not checked
(errcheck)
40-40: Error return value of os.Chdir is not checked
(errcheck)
tests/cli_terraform_test.go
191-191: use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$
(forbidigo)
🪛 GitHub Check: golangci
pkg/config/config_test.go
[failure] 28-28: [golangci] pkg/config/config_test.go#L28
Error return value of f.WriteString is not checked (errcheck)
[failure] 29-29: [golangci] pkg/config/config_test.go#L29
Error return value of f.WriteString is not checked (errcheck)
[failure] 30-30: [golangci] pkg/config/config_test.go#L30
Error return value of f.WriteString is not checked (errcheck)
[failure] 31-31: [golangci] pkg/config/config_test.go#L31
Error return value of f.WriteString is not checked (errcheck)
[failure] 38-38: [golangci] pkg/config/config_test.go#L38
Error return value of os.Chdir is not checked (errcheck)
[failure] 40-40: [golangci] pkg/config/config_test.go#L40
Error return value of os.Chdir is not checked (errcheck)
[failure] 11-11: [golangci] pkg/config/config_test.go#L11
Comment should end in a period (godot)
tests/cli_terraform_test.go
[failure] 191-191: [golangci] tests/cli_terraform_test.go#L191
use of fmt.Printf forbidden by pattern ^(fmt\.Print(|f|ln)|print|println)$ (forbidigo)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/config/config_test.go (1)
46-48:❓ Verification inconclusive
Test validates relative path despite PR objective
Your PR objective mentions ensuring ATMOS_BASE_PATH is always set to an absolute path, but this test is validating a relative path (.). Make sure this is intentional and consistent with how the configuration is meant to be used.
🏁 Script executed:
#!/bin/bash # Find how BasePath is used in the InitCliConfig function ast-grep --pattern $'func InitCliConfig($_, $_) { $$$ atmosConfig.BasePath = $_ $$$ }'Length of output: 93
Action Required: Confirm Configuration Path Expectation
The test in
pkg/config/config_test.go(lines 46-48) still verifies thatatmosConfig.BasePathis".", which implies a relative path. This appears to contradict the PR objective of always settingATMOS_BASE_PATHto an absolute path.
- Our initial search for how
atmosConfig.BasePathis assigned (viaInitCliConfig) did not return any matches.- Please manually verify if the current expectation (i.e., using a relative path
"."in the test) is intentional or if the assignment logic should be updated to enforce an absolute path.
There was a problem hiding this comment.
PR Overview
This pull request fixes the handling of ATMOS_CLI_CONFIG_PATH and ATMOS_BASE_PATH by ensuring the base path is always absolute and by updating the CLI config path logic.
- Ensure ATMOS_BASE_PATH is resolved to an absolute path when passed to downstream commands.
- Update ATMOS_CLI_CONFIG_PATH to be set as the directory of the configuration file.
- Update test cases and disable the atmos_vendor_pull test for consistency.
Reviewed Changes
| File | Description |
|---|---|
| pkg/config/config_test.go | Tests updated to validate absolute paths and CLI config directory. |
| tests/cli_terraform_test.go | Added environment variable tests and validated path updates. |
| pkg/config/config.go | Modified logic to extract and set the CLI config path from its file directory. |
| tests/test-cases/env.yaml | Updated expected regex patterns for base and config paths. |
| internal/exec/helmfile.go | Updated base path resolution to absolute before setting in ENV vars. |
| tests/test-cases/demo-stacks.yaml | Disabled atmos_vendor_pull test as part of cleanup. |
| internal/exec/terraform.go | Changed base path handling to absolute before setting in ENV vars. |
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/config/config_test.go (2)
12-12: Add period to comment for consistency.The comment before the test function should end with a period to follow the Go coding style conventions.
-// test base path and atmosConfigFilePath +// Test base path and atmosConfigFilePath.🧰 Tools
🪛 GitHub Check: golangci
[failure] 12-12: [golangci] pkg/config/config_test.go#L12
Comment should end in a period (godot)
13-81: Consider adding more test cases.The test currently only checks one scenario with a relative path
./. Consider adding test cases for:
- When
base_pathis set to an absolute path- When
base_pathis missing from the config- When
base_pathcontains environment variablesThis would provide more comprehensive test coverage for the functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/config_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci
pkg/config/config_test.go
[failure] 12-12: [golangci] pkg/config/config_test.go#L12
Comment should end in a period (godot)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/config/config_test.go (4)
28-33: LGTM - Proper string format with newlines.Good job on properly formatting the configuration file content with explicit newline characters. This ensures the YAML file is correctly formatted.
35-39: LGTM - Proper error handling.Good implementation of error handling when writing to the file. Each write operation now correctly checks for errors.
47-51: LGTM - Proper error handling in defer.Good implementation of error handling for changing the directory back in the defer function.
53-55: LGTM - Proper error handling for directory change.Good implementation of error handling when changing to the temporary directory.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/exec/helmfile_test.go (2)
35-35: 🛠️ Refactor suggestionFix misleading comment.
This comment is inconsistent with the actual function being tested.
- // set info for test ExecuteHelmfile + // set info for test ExecuteTerraform
12-15: 🛠️ Refactor suggestionInconsistency between comments and code function references.
The comment indicates this test checks the "helmfile apply command", but the test actually executes
ExecuteTerraform(). The PR is about fixing path handling for both Terraform and Helmfile, so make sure comments properly reflect the function being tested.-// TestExecuteTerraform_ExportEnvVar check that when executing the helmfile apply command. -// It checks that the environment variables are correctly exported and used. -// Env var `ATMOS_BASE_PATH` and `ATMOS_CLI_CONFIG_PATH` should be exported and used in the terraform apply command. +// TestExecuteTerraform_ExportEnvVar checks that when executing the terraform apply command, +// the environment variables are correctly exported and used. +// Env var `ATMOS_BASE_PATH` and `ATMOS_CLI_CONFIG_PATH` should be exported and used in the terraform apply command.
🧹 Nitpick comments (1)
internal/exec/helmfile_test.go (1)
64-69: Consider more robust string matching for environment variables.Current implementation checks for substring presence which could lead to false positives. Consider matching specific environment variable declarations.
- if !strings.Contains(output, "ATMOS_BASE_PATH") { + if !strings.Contains(output, "atmos_base_path=") { t.Errorf("ATMOS_BASE_PATH not found in the output") } - if !strings.Contains(output, "ATMOS_CLI_CONFIG_PATH") { + if !strings.Contains(output, "atmos_cli_config_path=") { t.Errorf("ATMOS_CLI_CONFIG_PATH not found in the output") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/exec/helmfile_test.go(1 hunks)tests/test-cases/demo-stacks.yaml(0 hunks)tests/test-cases/env.yaml(1 hunks)tests/test-cases/vendor-test.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test-cases/demo-stacks.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test-cases/env.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
tests/test-cases/vendor-test.yaml (1)
14-49: New Test Case Implementation: atmos_vendor_pullThis new test case, defined from lines 14 to 49, validates that the
atmos vendor pullcommand executes correctly when run from the specified working directory (fixtures/scenarios/vendor). It also checks the existence of multiple Terraform files in various subdirectories to ensure that the vendoring process populated the files as expected.Key observations:
- The test is properly disabled by default (
enabled: false), which is wise given that the OCI test repository adjustments (as noted by the commented-out lines) are still pending.- The
expectblock is comprehensive. It verifies several key file paths, and the TODO comments for the "my-vpc1" checks serve as a useful reminder for future improvements once the OCI repository issue is resolved.This addition aligns well with the PR objectives of standardizing how paths are handled and ensuring consistency in the behavior of Atmos across environments.
internal/exec/helmfile_test.go (4)
48-51: Good implementation consistent with test name.The test correctly calls
ExecuteTerraformwhich matches the test function name. This properly addresses the previously identified inconsistency issue.
71-80: Approve extraction and verification of environment variables.The implementation correctly extracts and verifies the presence of environment variables from the test output.
81-97: Strong validation of directory requirements.The test properly ensures that both
ATMOS_BASE_PATHandATMOS_CLI_CONFIG_PATHrefer to directories, which directly addresses the PR objectives to fix path handling. This is a solid implementation.
103-135: Well-designed helper function for extracting key-value pairs.The
extractKeyValuePairsfunction is clean, handles edge cases, and serves its purpose well. It correctly parses environment variables from the test output.
|
These changes were released in v1.164.0. |
What
This pull request (PR #1096) corrects the handling of
ATMOS_CLI_CONFIG_PATHandATMOS_BASE_PATHin Atmos by:ATMOS_BASE_PATHis always set to an absolute path.ATMOS_CLI_CONFIG_PATHto store the directory of the config file instead of the file path itself.atmos_vendor_pull) indemo-stacks.yaml.Why
ATMOS_BASE_PATHcould be set to a relative path, which could cause inconsistencies in resolving paths for components and stacks.ATMOS_CLI_CONFIG_PATHwas being stored as the file path rather than the directory, leading to incorrect assumptions about where the config should be read from.references
ATMOS_CLI_CONFIG_PATHset wrong for terraform commands #1094ATMOS_BASE_PATHenvironment variable should be set to absolute path for terraform commands #1095Summary by CodeRabbit
Refactor
Bug Fixes
atmos_vendor_pullto streamline testing focus.Tests
atmos vendor pullcommand.