Fix vendor pull directory creation issue #782
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
internal/exec/vendor_utils.go (1)
554-557: LGTM! This is a solid fix for the directory creation issue.The addition of
os.MkdirAllbefore the copy operation is exactly what was needed to resolve the "no such file or directory" error. The error handling is well-structured and provides clear feedback.A small suggestion to make the code even more robust:
targetDir := filepath.Dir(targetPath) -if err := os.MkdirAll(targetDir, 0755); err != nil { +if err := os.MkdirAll(targetDir, 0o755); err != nil { return fmt.Errorf("failed to create target directory '%s': %w", targetDir, err) }Using
0o755instead of0755makes the octal permission more explicit and follows modern Go conventions.
|
@Cerebrovinny I do not think this is the fix based on the description of the problem. Bear in mind we have two types of vendor manifests. https://atmos.tools/core-concepts/vendor/ The bug report concerns the component manifest file. That means that any logic relating to the vendor manifests file should not inhibit the component manifest from functioning. To me, it sounds like the regression is that when vendoring a component manifest, it is erroring if the vendor manifests file path does not exist. However, that should not be required. We should instead log to warning that the path does not exist and continue processing. We should not create the vendor configuration path if it does not exist. |
* fix: handle empty stack YAML configuration and adjust file content retrieval * fix: return existing content from GetFileContent instead of an empty string * fix: remove unnecessary blank lines in ProcessYAMLConfigFile and GetFileContent functions
* feat: set default Atmos manifest URL if not specified in configuration * feat: log trace message when using default Atmos JSON Schema file * fix: improve error message for missing Atmos JSON Schema file * Update internal/exec/validate_stacks.go * fix: change log level from trace to info for default Atmos JSON Schema message * fix: change log level from info to trace for default Atmos JSON Schema message --------- Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> Co-authored-by: Andriy Knysh <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
internal/exec/validate_stacks.go (1)
105-110: Consider improving error message readability.While the error message is informative, we can make it more readable.
Consider this format:
- return fmt.Errorf("Schema file '%s' not found. Configure via:\n"+ - "1. 'schemas.atmos.manifest' in atmos.yaml\n"+ - "2. ATMOS_SCHEMAS_ATMOS_MANIFEST env var\n"+ - "3. --schemas-atmos-manifest flag\n\n"+ - "Accepts: absolute path, paths relative to base_path, or URL", - cliConfig.Schemas.Atmos.Manifest) + return fmt.Errorf("Schema file '%s' not found.\n\n"+ + "Configure using any of these methods:\n"+ + " 1. 'schemas.atmos.manifest' in atmos.yaml\n"+ + " 2. ATMOS_SCHEMAS_ATMOS_MANIFEST env var\n"+ + " 3. --schemas-atmos-manifest flag\n\n"+ + "Accepts:\n"+ + " • absolute path\n"+ + " • paths relative to base_path\n"+ + " • URL", + cliConfig.Schemas.Atmos.Manifest)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/stack_processor_utils.go(1 hunks)internal/exec/validate_stacks.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/validate_stacks.go (1)
Learnt from: haitham911
PR: cloudposse/atmos#731
File: internal/exec/validate_stacks.go:0-0
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In `internal/exec/validate_stacks.go`, when downloading the Atmos JSON Schema file to the temp directory, the temporary file is overwritten each time, so explicit removal is not necessary.
🔇 Additional comments (4)
internal/exec/validate_stacks.go (2)
22-23: LGTM! Good addition of default schema URL.
Adding a default schema URL constant improves the robustness of the validation process. This ensures we always have a fallback schema location.
89-92: LGTM! Robust default schema handling.
The logic for handling empty schema configuration is solid. Using trace logging to inform about default schema usage is a good practice for debugging.
internal/exec/stack_processor_utils.go (2)
202-204: LGTM! The empty config handling looks good.
The addition of this early return for empty YAML configurations is a solid defensive programming practice. It gracefully handles the case where a file exists but is empty, returning empty maps which is consistent with the error handling pattern used when files don't exist.
202-204: Verify the impact on vendor pull operations.
While the empty config handling looks correct, let's verify that this change properly fixes the vendor pull directory creation issue.
#!/bin/bash
# Description: Check for any other instances where empty YAML configs might be handled differently
# Test: Search for similar empty YAML handling patterns
rg -A 5 "stackYamlConfig\s*=="
# Test: Look for vendor-related file operations
rg -A 5 "vendor.*pull"
|
@Cerebrovinny thanks for the PR.
|
|
@aknysh following test cases ✅ When vendor.yaml exists and using -c flag: ✅ When no vendor.yaml but component.yaml exists: ✅ When component specified but no vendor.yaml or component.yaml: ✅ When no component specified: b. Without vendor.yaml:
I also added tests integrating these criteria'sThis is before the fix so the tests fail
This is after the fix all tests are passing
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/vender/vendor_config_test.go (2)
14-27: Consider test setup improvementsA few suggestions to enhance the test setup:
- Consider using a less verbose log level for tests unless debugging
- Extract magic strings like "components/terraform" into constants for better maintainability
+const ( + defaultComponentPath = "components/terraform" + defaultLogLevel = "Info" // or "Error" for less verbose output +) cliConfig := schema.CliConfiguration{ BasePath: testDir, Components: schema.Components{ Terraform: schema.Terraform{ - BasePath: "components/terraform", + BasePath: defaultComponentPath, }, }, } -cliConfig.Logs.Level = "Trace" +cliConfig.Logs.Level = defaultLogLevel
28-32: Extract directory permissions to a constantConsider extracting the directory permissions to a named constant for better maintainability and clarity.
+const defaultDirPerms = 0755 componentPath := path.Join(testDir, "components", "terraform", "myapp") -err := os.MkdirAll(componentPath, 0755) +err := os.MkdirAll(componentPath, defaultDirPerms)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/vender/vendor_config_test.go(1 hunks)
🔇 Additional comments (1)
pkg/vender/vendor_config_test.go (1)
33-142: 🛠️ Refactor suggestion
Consider enhancing test coverage and structure
The test cases are comprehensive but could be improved:
- Consider using table-driven tests for YAML content variations
- Extract common setup/cleanup into helper functions
- Add edge cases mentioned in PR comments by osterman regarding component manifest handling
Here's a suggested refactor for the YAML content:
+var testCases = []struct {
+ name string
+ vendorYaml string
+ wantErr bool
+}{
+ {
+ name: "valid vendor config",
+ vendorYaml: `apiVersion: atmos/v1
+kind: AtmosVendorConfig
+metadata:
+ name: test-vendor-config
+spec:
+ sources:
+ - component: myapp
+ source: github.com/cloudposse/terraform-null-label.git//exports?ref={{.Version}}
+ version: 0.25.0`,
+ wantErr: false,
+ },
+ // Add more test cases
+}Also, consider adding these helper functions:
func setupVendorYaml(t *testing.T, dir, content string) string {
path := path.Join(dir, "vendor.yaml")
err := os.WriteFile(path, []byte(content), 0644)
assert.Nil(t, err)
return path
}
func cleanup(t *testing.T, paths ...string) {
for _, p := range paths {
err := os.Remove(p)
assert.Nil(t, err)
}
}Let's verify the test coverage aligns with PR objectives:
| package vender | ||
|
|
||
| import ( | ||
| "os" | ||
| "path" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| e "github.com/cloudposse/atmos/internal/exec" | ||
| "github.com/cloudposse/atmos/pkg/schema" | ||
| ) |
There was a problem hiding this comment.
Fix package name typo: 'vender' should be 'vendor'
The package name contains a typo that should be corrected for consistency.
-package vender
+package vendor📝 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.
| package vender | |
| import ( | |
| "os" | |
| "path" | |
| "testing" | |
| "github.com/stretchr/testify/assert" | |
| e "github.com/cloudposse/atmos/internal/exec" | |
| "github.com/cloudposse/atmos/pkg/schema" | |
| ) | |
| package vendor | |
| import ( | |
| "os" | |
| "path" | |
| "testing" | |
| "github.com/stretchr/testify/assert" | |
| e "github.com/cloudposse/atmos/internal/exec" | |
| "github.com/cloudposse/atmos/pkg/schema" | |
| ) |
There was a problem hiding this comment.
I also noticed that but I think its made by purpose right @aknysh ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
what
Fixed the regression where the vendor command would error if the vendor manifests file path did not exist.
why
Previously, the atmos vendor pull command would fail with a “no such file or directory” error when the vendor manifests file path was missing.
references
Closes #780
Logic design
@aknysh following test cases
✅ When vendor.yaml exists and using -c flag:
Correctly checks if the component exists in vendor.yaml
Error if component not found in vendor.yaml
✅ When no vendor.yaml but component.yaml exists:
Proceeds without vendor configurations
Pulls from component.yaml configuration
Shows a Warning about vendor.yaml not existing
✅ When component specified but no vendor.yaml or component.yaml:
System correct shows vendor.yaml doesn't exist
Properly errors that component.yaml doesn't exist in the component folder
✅ When no component specified:
a. With vendor.yaml:
System attempts to process vendor.yaml
Shows appropriate error for invalid configuration
b. Without vendor.yaml:
System shows vendor.yaml doesn't exist
Provides an error message about needing to specify --component flag
I also added tests integrating these criteria's
This is before the fix so the tests fail
This is after the fix all tests are passing
Summary by CodeRabbit
New Features
Bug Fixes
Tests