Config Core library #1174
Config Core library #1174
Conversation
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
dev-3093-create-a-cli-command-core-library
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
golangci-lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…ttps://github.com/cloudposse/atmos into feature/dev-3093-create-a-cli-command-core-library
…ttps://github.com/cloudposse/atmos into feature/dev-3093-create-a-cli-command-core-library
samtholiya
force-pushed
the
feature/dev-3093-create-a-cli-command-core-library
branch
from
115a9de to
247ab4c
Compare
…ttps://github.com/cloudposse/atmos into feature/dev-3093-create-a-cli-command-core-library
samtholiya
force-pushed
the
feature/dev-3093-create-a-cli-command-core-library
branch
from
ade57f1 to
92b4c4e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…mand-core-library
Benbentwo
force-pushed
the
feature/dev-3093-create-a-cli-command-core-library
branch
from
c8f109d to
037fb3e
Compare
|
Hey @samtholiya whats the purpose of the IIRC Viper + Cobra had a way to play nicely together. This still feels pretty complex to me. (Also I force pushed to reset to the merge and created a branch from there to test some changes) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/yaml_func_terraform_output_test.go (4)
53-57: Consider making fixture path more resilient.The hard-coded path to the test fixture could be problematic if directory structures change in the future.
// Define the working directory -workDir := "./fixtures/scenarios/atmos-terraform-output-yaml-function" +workDir := filepath.Join("fixtures", "scenarios", "atmos-terraform-output-yaml-function") if err := os.Chdir(workDir); err != nil { t.Fatalf("Failed to change directory to %q: %v", workDir, err) }
69-83: Consider adding more detailed assertions.The test verifies key presence in the output but could be more specific about expected values.
assert.Contains(t, y, "foo: component-1-a") assert.Contains(t, y, "bar: component-1-b") assert.Contains(t, y, "baz: component-1-c") +// Consider using more specific assertions to verify exact structure +expectedValues := map[string]string{ + "foo": "component-1-a", + "bar": "component-1-b", + "baz": "component-1-c", +} +for k, v := range expectedValues { + assert.Contains(t, y, fmt.Sprintf("%s: %s", k, v)) +}
16-16: Add a comment describing the test purpose.Adding a descriptive comment would improve documentation and help future maintainers understand the test's purpose.
+// TestYamlFuncTerraformOutput validates that Terraform output YAML functions work correctly by +// deploying components with templates and function processing enabled, then verifying the output values. func TestYamlFuncTerraformOutput(t *testing.T) {
40-41: Consider extracting cleanup paths to variables.The hardcoded relative paths used for cleanup could be made more maintainable.
+// Define paths relative to the fixture for cleanup +terraformDir := filepath.Join("..", "..", "components", "terraform", "mock") +terraformStateDir := filepath.Join(terraformDir, ".terraform") +terraformStateDDir := filepath.Join(terraformDir, "terraform.tfstate.d") defer func() { // Delete the generated files and folders after the test - err := os.RemoveAll(filepath.Join("..", "..", "components", "terraform", "mock", ".terraform")) + err := os.RemoveAll(terraformStateDir) assert.NoError(t, err) - err = os.RemoveAll(filepath.Join("..", "..", "components", "terraform", "mock", "terraform.tfstate.d")) + err = os.RemoveAll(terraformStateDDir) assert.NoError(t, err)Also applies to: 43-44
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/yaml_func_terraform_output_test.go(0 hunks)tests/yaml_func_terraform_output_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/exec/yaml_func_terraform_output_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
tests/yaml_func_terraform_output_test.go (4)
17-25: Good environment setup for isolated testing.The test properly unsets environment variables to ensure a clean testing environment. This prevents any external configuration from affecting test results.
38-51: Strong cleanup practice with defer.Good use of defer to ensure proper cleanup after test execution. This guarantees that temporary files are removed and the original working directory is restored regardless of test outcome.
59-68: Good test execution pattern with proper error handling.The test properly simulates CLI execution by setting os.Args and calling cmd.Execute(). This approach tests the entire execution flow, which is valuable for integration testing.
1-84: Overall solid integration test.This test effectively validates the integration of Terraform output YAML functions within the Atmos CLI tool. It follows good practices for environment setup, test execution, and cleanup.
The test properly simulates real CLI usage and verifies the correctness of Terraform output YAML conversion, which aligns with the PR's goal of improving configuration handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/terraform_test.go (1)
43-48: Consider more specific error messages.When checking if the planfile exists, consider providing more context in the error message to help debugging failures.
- t.Errorf("Generated planfile does not exist: %s", filePath) + t.Errorf("Generated planfile does not exist at %s - verify the command properly created output files", filePath) - t.Errorf("Error checking file: %v", err) + t.Errorf("Error checking generated planfile at %s: %v", filePath, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/terraform_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/terraform_test.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
🧬 Code Graph Analysis (1)
tests/terraform_test.go (3)
pkg/config/config_new.go (1)
DefaultConfigHandler(14-14)pkg/utils/markdown_utils.go (1)
PrintErrorMarkdownAndExitFn(24-24)cmd/cmd_utils.go (1)
Contains(780-787)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
tests/terraform_test.go (7)
11-11: Correct import alias usage for charmbracelet/log.The import alias for
github.com/charmbracelet/logis correctly set aslog, which follows the project's established import conventions.
18-49: Well-structured new test for terraform planfile generation.This new test appropriately validates the CLI command for generating planfiles by:
- Setting up test fixtures
- Initializing the configuration handler
- Executing the command through the CLI interface
- Verifying the generated file exists
- Cleaning up test artifacts
The approach follows modern testing practices by testing at the CLI level rather than internal function calls.
65-66: Path refactoring improves code organization.The change from relative paths starting with
../../tests/fixturesto shorter./fixturespaths indicates improved test organization, with test files likely moved closer to the fixtures they use.Also applies to: 120-120, 176-176, 232-232, 257-257, 314-314
75-76: CLI-focused testing approach is more comprehensive.The shift from direct function calls to CLI command execution via
cmd.Execute()with appropriateos.Argsprovides more thorough end-to-end testing. This ensures that command registration, flag parsing, and execution work correctly in the actual CLI environment.Also applies to: 129-130, 185-186, 237-238, 268-269, 323-324
210-219: Improved error handling verification.The test now properly verifies error handling by:
- Setting the debug log level
- Mocking the error handling function
- Checking that the error function is called with the expected error message
This approach ensures the command properly reports errors rather than just checking for function return values.
Also applies to: 226-230, 239-239
295-378: Comprehensive environment variable verification.The TestExecuteTerraform_ExportEnvVar test effectively:
- Executes the command in a controlled directory
- Captures the command output
- Extracts and validates environment variables
- Verifies that paths are valid directories
This provides important validation of the environment variable handling in the CLI commands.
359-366: Robust path validation enhances test reliability.The verification that ATMOS_BASE_PATH and ATMOS_CLI_CONFIG_PATH are valid directories adds a layer of validation beyond just checking for their presence. This helps catch potential issues where these variables might be set but point to invalid locations.
Also applies to: 369-375
| workDir := "../../tests/fixtures/scenarios/env" | ||
| if err := os.Chdir(workDir); err != nil { | ||
| t.Fatalf("Failed to change directory to %q: %v", workDir, err) | ||
| config.DefaultConfigHandler = config.New() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider test isolation for the global config handler.
The test initializes the global DefaultConfigHandler which could potentially affect other tests. While this approach works for the CLI testing pattern, it reinforces the concerns raised in PR comments about global state management.
🏁 Script executed:
#!/bin/bash
# Check how many files use the DefaultConfigHandler directly in tests
rg -l "DefaultConfigHandler = " --type go ./testsLength of output: 77
Isolate global DefaultConfigHandler in tests
To prevent one test from unintentionally affecting others, capture and restore the original handler around your override:
- File: tests/terraform_test.go, line 36
Wrap your assignment as follows:
- config.DefaultConfigHandler = config.New()
+ oldHandler := config.DefaultConfigHandler
+ defer func() { config.DefaultConfigHandler = oldHandler }()
+ config.DefaultConfigHandler = config.New()This ensures the global state is reset once the test completes.
📝 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.
| config.DefaultConfigHandler = config.New() | |
| oldHandler := config.DefaultConfigHandler | |
| defer func() { config.DefaultConfigHandler = oldHandler }() | |
| config.DefaultConfigHandler = config.New() |
🤖 Prompt for AI Agents (early access)
In tests/terraform_test.go at line 36, the test directly overrides the global
DefaultConfigHandler which can cause side effects on other tests. To fix this,
save the original DefaultConfigHandler before assigning the new one, and defer
restoring it after the test finishes. This ensures the global state is isolated
and reset properly, preventing interference between tests.
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
|
This PR was closed due to inactivity and merge conflicts. 😭 |
mergify
bot
removed
conflict
what
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores