Added Support for Automatic Templated File Search for Atmos Imports #795
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the handling of file imports and configuration searches in the Atmos system. Modifications include improved logic for recognizing multiple YAML file extensions, including templated versions, and refined error handling for file operations. New helper functions have been introduced to streamline file matching processes, contributing to a more robust configuration management system. Changes
Assessment against linked issues
Possibly related 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: 3
🧹 Outside diff range and nitpick comments (4)
pkg/utils/file_utils.go (2)
42-48: LGTM! Clean implementation of template extension support.The implementation efficiently handles both regular YAML and templated YAML files while maintaining backward compatibility. The extension check is particularly well-done, properly handling compound extensions.
Consider updating the function comment to reflect template support:
-// IsYaml checks if the file has YAML extension (does not check file schema, nor validates the file) +// IsYaml checks if the file has YAML extension, including template extensions (.yaml.tmpl, .yml.tmpl). +// Note: This function does not check file schema, nor validates the file content.
193-193: Consider optimizing extension order for performance.Since the PR objectives mention falling back to regular YAML files when templates aren't found, consider ordering the extensions to check regular files first:
-configExtensions := []string{".yaml", ".yml", ".yaml.tmpl", ".yml.tmpl"} +// Check regular files first, then templates +configExtensions := []string{".yml", ".yaml", ".yml.tmpl", ".yaml.tmpl"}Additionally, consider declaring these extensions as package-level constants since they're used in multiple functions:
const ( YamlExt = ".yaml" YmlExt = ".yml" YamlTmplExt = ".yaml.tmpl" YmlTmplExt = ".yml.tmpl" ) var configExtensions = []string{YmlExt, YamlExt, YmlTmplExt, YamlTmplExt}internal/exec/stack_processor_utils.go (1)
393-397: Consider adding error logging for template file access issues.While the template file check is correct, it might be helpful to log when there are permission issues accessing the template file, to aid in troubleshooting.
if ext == ".yaml" || ext == ".yml" { // Check if there's a template version of this file templatePath := impWithExt + ".tmpl" - if _, err := os.Stat(path.Join(basePath, templatePath)); err == nil { + if _, err := os.Stat(path.Join(basePath, templatePath)); err == nil { impWithExt = templatePath + } else if !os.IsNotExist(err) { + // Log template access error but continue with non-template version + u.LogWarning("Error accessing template file %s: %v", templatePath, err) } }pkg/config/utils.go (1)
716-717: Enhance error message for clarityImprove the error message to provide more context when no match is found.
Suggestion:
- return "", fmt.Errorf("failed to find a match for the import '%s' ('%s' + '%s')", configPath, dir, base) + return "", fmt.Errorf("failed to find a matching config file for '%s' in directory '%s'", base, dir)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
internal/exec/stack_processor_utils.go(1 hunks)pkg/config/utils.go(4 hunks)pkg/utils/file_utils.go(2 hunks)
🔇 Additional comments (3)
pkg/utils/file_utils.go (1)
42-48: Verify impact on existing YAML file operations.
The changes to YAML extension handling affect multiple functions. Let's verify the impact:
Also applies to: 193-193
✅ Verification successful
Changes to YAML extension handling are safe and well-contained
The changes to IsYaml function are well-contained and properly handle all YAML extensions. The function is used in two places:
GetAllYamlFilesInDirwhich uses it to filter YAML files- Stack processor which uses it to identify YAML configurations
The updated implementation correctly handles:
- Standard YAML extensions (.yaml, .yml)
- Template extensions (.yaml.tmpl, .yml.tmpl)
- Proper extension extraction for template files
The changes maintain backward compatibility while adding proper support for template files. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts on YAML file handling across the codebase
# Find all functions that use IsYaml or handle YAML files
echo "Functions potentially affected by the changes:"
rg -A 2 "IsYaml|\.ya?ml" --type go
# Find all test files that verify YAML handling
echo -e "\nTest coverage for YAML handling:"
fd ".*_test\.go" --exec rg -l "IsYaml|\.ya?ml"
Length of output: 35075
Script:
#!/bin/bash
# Let's check for all usages of IsYaml function and analyze its implementation
ast-grep --pattern 'IsYaml($$$)'
# Also check for direct YAML extension checks in the codebase
rg -A 2 'filepath\.Ext.*\.ya?ml' --type go
Length of output: 124
Script:
#!/bin/bash
# Let's check for all functions that use IsYaml and analyze the call sites
rg -A 3 "IsYaml\(" --type go
# Also check for all functions that use this YAML extension list
rg -A 3 "yamlExtensions.*:=.*\[\]string" --type go
Length of output: 1038
internal/exec/stack_processor_utils.go (2)
371-392: LGTM! The extension handling logic is well-structured.
The implementation provides a robust fallback mechanism for file extensions, checking for both standard and templated versions in a prioritized order. The code is clear and maintainable.
371-397: Verify the impact on existing template file handling.
The new extension handling logic modifies how template files are discovered. Let's verify this doesn't affect existing template file imports.
✅ Verification successful
Template file handling changes are safe and consistent
The new extension handling logic aligns perfectly with existing template file handling patterns across the codebase. The search results show that:
- The same extension patterns (
.yaml.tmpl,.yml.tmpl) are consistently used infile_utils.goandconfig/utils.go - The new code follows the established precedent of checking for both regular and template versions of YAML files
- No direct import statements with hardcoded template extensions were found, indicating flexible extension handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing template file usage patterns that might be affected
# Test: Look for current template file imports
rg -l '\.ya?ml\.tmpl'
# Test: Check for any import statements referencing template files
ast-grep --pattern 'import: $_\.ya?ml\.tmpl'
Length of output: 199
Script:
#!/bin/bash
# Let's examine the actual usage patterns in the identified files
# Check file_utils.go for template handling
echo "=== pkg/utils/file_utils.go ==="
rg -A 5 -B 5 '\.ya?ml\.tmpl' pkg/utils/file_utils.go
# Check config utils for template handling
echo "=== pkg/config/utils.go ==="
rg -A 5 -B 5 '\.ya?ml\.tmpl' pkg/config/utils.go
# Look for actual import statements with yaml/yml extensions
echo "=== Checking import statements ==="
rg -A 2 'import:.*\.ya?ml'
# Check for any template-related functions
echo "=== Template-related functions ==="
ast-grep --pattern 'func $_($$$ template $$$) {
$$$
}'
Length of output: 2945
|
Please update the docs here: https://atmos.tools/core-concepts/stacks/imports#go-templates-in-imports |
| if err != nil { | ||
| if cliConfig.Logs.Level == u.LogLevelTrace { | ||
| y, _ := u.ConvertToYAML(cliConfig) | ||
| return nil, nil, fmt.Errorf("%v\n\n\nCLI config:\n\n%v", err, y) | ||
| } | ||
| return nil, nil, err | ||
| } | ||
| continue |
There was a problem hiding this comment.
if allMatches is empty, u.GetGlobMatches(patterns[0] gets called.
Are we checking just for an error and why?
Please review the logic, or add comments above the continue to explain the case in more details
There was a problem hiding this comment.
@aknysh ok good point, this happens sometimes If there's no error but still no matches, so we continue to the next path
e.g when the pattern is valid but no files match it
I have added detailed code explaining it
pkg/utils/file_utils.go
Outdated
| @@ -39,8 +39,13 @@ func FileOrDirExists(filename string) bool { | |||
|
|
|||
| // IsYaml checks if the file has YAML extension (does not check file schema, nor validates the file) | |||
| func IsYaml(file string) bool { | |||
| yamlExtensions := []string{".yaml", ".yml"} | |||
| yamlExtensions := []string{".yaml", ".yml", ".yaml.tmpl", ".yml.tmpl"} | |||
There was a problem hiding this comment.
these ".yaml", ".yml", ".yaml.tmpl", ".yml.tmpl" can be global constants, please see the comment above
There was a problem hiding this comment.
done clean up
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/config/utils.go (2)
663-684: Define constants for file extensionsThe function is well-implemented with proper empty path validation. However, the file extensions should be defined as constants to improve maintainability.
+const ( + TemplateExt = ".tmpl" + YamlTmplExt = ".yaml.tmpl" + YmlTmplExt = ".yml.tmpl" +) func getConfigFilePatterns(path string, forGlobMatch bool) []string { // ... existing code ... patterns := []string{ - path + DefaultStackConfigFileExtension, - path + DefaultStackConfigFileExtension + ".tmpl", - path + ".yaml.tmpl", - path + ".yml.tmpl", + path + DefaultStackConfigFileExtension, + path + DefaultStackConfigFileExtension + TemplateExt, + path + YamlTmplExt, + path + YmlTmplExt, }
686-720: Add function documentationThe implementation is solid with proper error handling and efficient file lookup. Consider adding documentation to explain the function's behavior and parameters.
+// SearchConfigFile searches for a configuration file in the specified path. +// It supports both direct file paths and base paths without extensions. +// For base paths, it tries different extensions in a predefined order. +// Parameters: +// configPath: The path to search for the configuration file +// cliConfig: The CLI configuration containing settings +// Returns: +// The full path to the found configuration file, or an error if not found func SearchConfigFile(configPath string, cliConfig schema.CliConfiguration) (string, error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/config/utils.go(4 hunks)
🔇 Additional comments (1)
pkg/config/utils.go (1)
109-111: LGTM: Clear error message for no matches
The error message clearly indicates when no matches are found for a stack, which improves debugging experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
internal/exec/workflow.go (3)
89-89: LGTM on the extension source change, but consider enhancing for template support.The move to use
u.DefaultStackConfigFileExtensionaligns with the codebase's centralization of file extension handling. However, to fulfill the PR's objectives for automatic templated file detection, we should enhance this section.Consider updating the extension handling to support templated files:
ext = u.DefaultStackConfigFileExtension -workflowPath = workflowPath + ext + +// First try with .tmpl extension +templatePath := workflowPath + ext + ".tmpl" +if u.FileExists(templatePath) { + workflowPath = templatePath +} else { + // Fallback to non-templated file + workflowPath = workflowPath + ext +}
Line range hint
93-96: Consider enhancing error message for template context.The error message could be more helpful by indicating the attempted paths, including the templated version.
Consider updating the error message:
-return fmt.Errorf("the workflow manifest file '%s' does not exist", workflowPath) +return fmt.Errorf("workflow manifest not found at '%s' or '%s.tmpl'", workflowPath, workflowPath)
Line range hint
98-116: Consider template processing for file content.If we're supporting templated files, we should ensure the content is processed through the template engine when the file has a
.tmplextension.Consider adding template processing:
fileContent, err := os.ReadFile(workflowPath) if err != nil { return err } + +// Process template if file ends with .tmpl +if strings.HasSuffix(workflowPath, ".tmpl") { + fileContent, err = u.ProcessTemplate(fileContent) + if err != nil { + return fmt.Errorf("failed to process template %s: %w", workflowPath, err) + } +}pkg/utils/file_utils.go (1)
188-193: Strong implementation of the fallback mechanism!The function effectively implements the fallback search for config files, aligning well with the PR objectives for automatic templated file detection. It gracefully handles both explicit paths and extension-less paths.
Once the shared
yamlExtensionsvariable is created (from the previous comment), update this section to use it:-configExtensions := []string{YamlFileExtension, YmlFileExtension, YamlTemplateExtension, YmlTemplateExtension} -for _, ext := range configExtensions { +for _, ext := range yamlExtensions {cmd/cmd_utils.go (1)
Line range hint
258-273: Consider enhancing error messages for better template support visibility.The error messages could be more descriptive about the template support capabilities. Consider adding information about the supported file extensions (
.yaml.tmpl,.yml.tmpl) in the error messages to help users understand the available options.Here's a suggested improvement:
- commandConfig.ComponentConfig.Component, cfg.CliConfigFileName+u.DefaultStackConfigFileExtension)) + commandConfig.ComponentConfig.Component, fmt.Sprintf("%s%s (or %s.tmpl)", cfg.CliConfigFileName, u.DefaultStackConfigFileExtension, cfg.CliConfigFileName+u.DefaultStackConfigFileExtension)))Similar change can be applied to the stack error message as well.
pkg/config/utils.go (2)
67-70: Simplify stack matching logic using getConfigFilePatternsThe multiple suffix checks can be simplified by leveraging the patterns from getConfigFilePatterns.
-stackMatch := strings.HasSuffix(matchedFileAbsolutePath, stack+u.DefaultStackConfigFileExtension) || - strings.HasSuffix(matchedFileAbsolutePath, stack+u.DefaultStackConfigFileExtension+u.TemplateExtension) || - strings.HasSuffix(matchedFileAbsolutePath, stack+u.YamlTemplateExtension) || - strings.HasSuffix(matchedFileAbsolutePath, stack+u.YmlTemplateExtension) +patterns := getConfigFilePatterns(stack, false) +stackMatch := false +for _, pattern := range patterns { + if strings.HasSuffix(matchedFileAbsolutePath, pattern) { + stackMatch = true + break + } +}
686-720: Consider performance optimization for large directoriesThe current implementation reads all directory entries into memory. For very large directories, this could be optimized by checking file existence directly for each pattern instead.
- // Create a map of existing files for quick lookup - fileMap := make(map[string]bool) - for _, entry := range entries { - if !entry.IsDir() { - fileMap[entry.Name()] = true - } - } - - // Try all patterns in order - patterns := getConfigFilePatterns(base, false) - for _, pattern := range patterns { - if fileMap[pattern] { - return filepath.Join(dir, pattern), nil - } - } + // Try patterns directly + patterns := getConfigFilePatterns(base, false) + for _, pattern := range patterns { + path := filepath.Join(dir, pattern) + if _, err := os.Stat(path); err == nil { + return path, nil + } + }internal/exec/stack_processor_utils.go (3)
374-394: Consider defining extensions as constants.The extension handling logic is well-structured, but the extension strings could be defined as package-level constants to improve maintainability and reusability.
+const ( + YamlExt = ".yaml" + YmlExt = ".yml" + YamlTmplExt = ".yaml.tmpl" + YmlTmplExt = ".yml.tmpl" +) extensions := []string{ - ".yaml", - ".yml", - ".yaml.tmpl", - ".yml.tmpl", + YamlExt, + YmlExt, + YamlTmplExt, + YmlTmplExt, }
395-400: Consider adding error handling for edge cases.While the template file checking works correctly, it might benefit from explicit error handling for edge cases such as file permission issues or system errors.
} else if ext == ".yaml" || ext == ".yml" { // Check if there's a template version of this file templatePath := impWithExt + ".tmpl" - if _, err := os.Stat(path.Join(basePath, templatePath)); err == nil { + if info, err := os.Stat(path.Join(basePath, templatePath)); err == nil { + // Verify file permissions and type + if info.Mode().IsRegular() && info.Mode().Perm()&0444 != 0 { + impWithExt = templatePath + } + } else if !os.IsNotExist(err) { + // Handle system errors + return nil, nil, nil, fmt.Errorf("error checking template file: %w", err) + } - impWithExt = templatePath } }
1887-1888: Consider extracting extension handling to a helper function.The extension stripping logic is duplicated. Consider extracting it to a helper function to improve maintainability and reduce duplication.
+// stripStackExtension removes the default stack config file extension from a stack name +func stripStackExtension(stack string) string { + return strings.Replace(stack, u.DefaultStackConfigFileExtension, "", 1) +} for stack, components := range stackComponentMap["terraform"] { for _, component := range components { - componentStackMap["terraform"][component] = append(componentStackMap["terraform"][component], strings.Replace(stack, u.DefaultStackConfigFileExtension, "", 1)) + componentStackMap["terraform"][component] = append(componentStackMap["terraform"][component], stripStackExtension(stack)) } } for stack, components := range stackComponentMap["helmfile"] { for _, component := range components { - componentStackMap["helmfile"][component] = append(componentStackMap["helmfile"][component], strings.Replace(stack, u.DefaultStackConfigFileExtension, "", 1)) + componentStackMap["helmfile"][component] = append(componentStackMap["helmfile"][component], stripStackExtension(stack)) } }Also applies to: 1893-1894
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
cmd/cmd_utils.go(2 hunks)internal/exec/stack_processor_utils.go(5 hunks)internal/exec/workflow.go(1 hunks)pkg/config/const.go(1 hunks)pkg/config/utils.go(4 hunks)pkg/utils/file_extensions.go(1 hunks)pkg/utils/file_utils.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/config/const.go
- pkg/utils/file_extensions.go
🧰 Additional context used
📓 Learnings (1)
internal/exec/stack_processor_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
🔇 Additional comments (9)
internal/exec/workflow.go (1)
Line range hint 89-116: Verify template handling consistency across the codebase.
Let's ensure this template handling approach is consistent with other file processing in Atmos.
pkg/utils/file_utils.go (2)
42-48: 🛠️ Refactor suggestion
Consider defining extension constants and reducing duplication.
The extension handling looks solid, but there are a couple of improvements we can make:
- The referenced constants (
YamlFileExtension, etc.) appear to be undefined in this file. - The extension list is duplicated in
SearchConfigFile.
Consider:
- Define the constants at the package level:
const (
YamlFileExtension = ".yaml"
YmlFileExtension = ".yml"
YamlTemplateExtension = ".yaml.tmpl"
YmlTemplateExtension = ".yml.tmpl"
)- Create a shared variable to avoid duplication:
var yamlExtensions = []string{YamlFileExtension, YmlFileExtension, YamlTemplateExtension, YmlTemplateExtension}42-48: Verify template fallback behavior across the codebase.
The implementation looks solid, but let's verify the usage patterns across the codebase to ensure consistent behavior.
Also applies to: 188-193
✅ Verification successful
Template extension handling is consistent across the codebase
The implementation aligns with the codebase's usage patterns:
- File extensions are centrally defined in
pkg/utils/file_extensions.go - The
.yaml.tmpland.yml.tmplextensions are consistently used throughout - The fallback mechanism for
.yamland.ymlis properly supported - No conflicting extension handling patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of YAML extensions and potential edge cases
# Check for any hardcoded extensions that might need updating
echo "Checking for hardcoded YAML extensions..."
rg -n "\.ya?ml(\.tmpl)?" --type go
# Look for potential template-related patterns
echo -e "\nChecking for template-related patterns..."
ast-grep --pattern 'if $_ == ".tmpl" { $$$$ }' --lang go
# Check for other file extension checks that might need similar updates
echo -e "\nChecking for other file extension checks..."
ast-grep --pattern 'filepath.Ext($_)' --lang go
Length of output: 19131
cmd/cmd_utils.go (1)
261-261: LGTM! The changes align with the PR objectives.
The modification to use u.DefaultStackConfigFileExtension from the utils package is consistent with the PR's goal of enhancing templated file support. This change maintains backward compatibility while supporting the new automatic file detection feature.
Let's verify the consistency of file extension usage across the codebase:
Also applies to: 271-271
✅ Verification successful
The file extension usage is consistent and properly aligned with the codebase standards
The verification shows that:
- All references to
DefaultStackConfigFileExtensionare properly using the utils package version (u.DefaultStackConfigFileExtension) - The hardcoded
.yaml.tmpland.yml.tmplextensions only appear in the utils package as constants (YamlTemplateExtensionandYmlTemplateExtension) and in supporting validation logic - The changes in
cmd_utils.gofollow the established pattern of extension handling across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of DefaultStackConfigFileExtension
# Expected: All references should use the utils package version
# Search for any remaining direct usage of DefaultStackConfigFileExtension
rg -l "DefaultStackConfigFileExtension" --type go
# Check for any hardcoded .yaml.tmpl or .yml.tmpl extensions
rg -l "\.(yaml|yml)\.tmpl" --type go
Length of output: 311
Script:
#!/bin/bash
# Let's examine the actual usage to ensure consistency
# Check DefaultStackConfigFileExtension in utils package first
rg "DefaultStackConfigFileExtension" pkg/utils/file_extensions.go -A 2
# Then check how it's used in other files
rg "DefaultStackConfigFileExtension" pkg/config/utils.go internal/exec/workflow.go cmd/cmd_utils.go internal/exec/stack_processor_utils.go
# Check the hardcoded extensions to see their context
rg "\.(yaml|yml)\.tmpl" internal/exec/stack_processor_utils.go pkg/utils/file_utils.go pkg/utils/file_extensions.go
Length of output: 2603
pkg/config/utils.go (4)
109-111: LGTM! Clear and informative error handling
The error message provides good context by including both the stack name and search paths.
159-159: Add comment explaining the continue statement
As requested in previous reviews, please add a comment explaining when this continue statement is executed and why it's necessary.
663-684: LGTM! Well-structured helper function
The function handles both glob and direct file search patterns effectively, with proper input validation.
33-38: 🛠️ Refactor suggestion
Consider using getConfigFilePatterns helper function
The pattern array definition is duplicated across the codebase. Let's utilize the new helper function for consistency and maintainability.
-patterns = []string{
- p + u.DefaultStackConfigFileExtension,
- p + u.DefaultStackConfigFileExtension + u.TemplateExtension,
- p + u.YamlTemplateExtension,
- p + u.YmlTemplateExtension,
-}
+patterns = getConfigFilePatterns(p, true)Likely invalid or redundant comment.
internal/exec/stack_processor_utils.go (1)
480-481: LGTM! Default extension handling is correct.
The fallback to DefaultStackConfigFileExtension maintains backward compatibility and follows the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
internal/exec/stack_processor_utils.go (3)
374-379: Consider using constants for file extensions.The extension list could be defined as package-level constants for better maintainability and reusability.
+const ( + YamlExt = ".yaml" + YmlExt = ".yml" + TmplExt = ".tmpl" +) -extensions := []string{ - u.YamlFileExtension, - u.YmlFileExtension, - u.YamlTemplateExtension, - u.YmlTemplateExtension, -} +extensions := []string{ + YamlExt, + YmlExt, + YamlExt + TmplExt, + YmlExt + TmplExt, +}
395-400: Add documentation for template version detection logic.This section implements a key feature for template detection, but could benefit from a comment explaining the logic and fallback behavior.
+// Check for a template version of standard YAML files. +// If a .tmpl version exists (e.g., config.yaml.tmpl), use it instead +// of the standard version (config.yaml). This implements automatic +// template detection while maintaining backward compatibility. } else if ext == ".yaml" || ext == ".yml" { // Check if there's a template version of this file templatePath := impWithExt + ".tmpl"
1887-1887: Consider extracting extension handling to a helper function.The extension stripping logic is duplicated. Consider creating a helper function to maintain consistency and reduce duplication.
+// stripStackExtension removes the default stack configuration file extension +func stripStackExtension(filename string) string { + return strings.Replace(filename, u.DefaultStackConfigFileExtension, "", 1) +} -componentStackMap["terraform"][component] = append(componentStackMap["terraform"][component], strings.Replace(stack, u.DefaultStackConfigFileExtension, "", 1)) +componentStackMap["terraform"][component] = append(componentStackMap["terraform"][component], stripStackExtension(stack)) -componentStackMap["helmfile"][component] = append(componentStackMap["helmfile"][component], strings.Replace(stack, u.DefaultStackConfigFileExtension, "", 1)) +componentStackMap["helmfile"][component] = append(componentStackMap["helmfile"][component], stripStackExtension(stack))Also applies to: 1893-1893
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/stack_processor_utils.go(5 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/stack_processor_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
🔇 Additional comments (1)
internal/exec/stack_processor_utils.go (1)
374-400: Implementation successfully meets PR objectives!
The changes effectively implement automatic templated file detection while maintaining backward compatibility. The code:
- Supports both
.yaml.tmpland.yml.tmplextensions - Provides fallback to standard
.yamland.ymlfiles - Maintains backward compatibility with existing configurations
- Uses proper error handling and path manipulation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
website/docs/core-concepts/stacks/imports.mdx (3)
55-71: Enhance the documentation with additional clarity and examples.The new section effectively explains the automatic template detection feature. Consider these enhancements to make it even more helpful:
- Add an explicit example showing the fallback behavior:
For example, if you import `catalog/file1`, Atmos will: 1. First look for `catalog/file1.yaml` or `catalog/file1.yml` 2. If found, check if a template version exists (`catalog/file1.yaml.tmpl` or `catalog/file1.yml.tmpl`) 3. Use the template version if it exists, otherwise use the regular YAML file 4. If no files are found, default to using `.yaml` extension + +Example scenarios: +- If both `file1.yaml` and `file1.yaml.tmpl` exist, the `.tmpl` version is used +- If only `file1.yaml` exists, it's used directly +- If no matching files exist, Atmos will attempt to use `file1.yaml`
- Add a note about backward compatibility:
This feature makes it easier to work with templated configurations as you don't need to explicitly specify the template file extension - Atmos will automatically use the template version when available. + +:::note +This feature maintains backward compatibility with existing configurations. Explicitly specified file extensions (including `.tmpl`) will continue to work as before. +:::🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: Possible missing comma found.
Context: ... makes it easier to work with templated configurations as you don't need to explicitly specify...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~70-~70: To join two clauses or introduce examples, consider using an em dash.
Context: ...itly specify the template file extension - Atmos will automatically use the templat...(DASH_RULE)
55-55: Improve section connectivity with cross-references.Consider adding a reference to this new feature in the "Go Templates in Imports" section to help users understand the relationship between automatic template detection and manual template configuration:
### Automatic Template File Detection + +:::tip +This feature complements the manual template configuration described in the [Go Templates in Imports](#go-templates-in-imports) section below. While automatic detection simplifies common use cases, you can still explicitly specify template files when needed. +:::
70-70: Refine punctuation for improved readability.Consider these grammatical improvements:
-This feature makes it easier to work with templated configurations as you don't need to explicitly specify the template file extension - Atmos will automatically use the template version when available. +This feature makes it easier to work with templated configurations, as you don't need to explicitly specify the template file extension — Atmos will automatically use the template version when available.🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: Possible missing comma found.
Context: ... makes it easier to work with templated configurations as you don't need to explicitly specify...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~70-~70: To join two clauses or introduce examples, consider using an em dash.
Context: ...itly specify the template file extension - Atmos will automatically use the templat...(DASH_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
website/docs/core-concepts/stacks/imports.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/imports.mdx
[uncategorized] ~70-~70: Possible missing comma found.
Context: ... makes it easier to work with templated configurations as you don't need to explicitly specify...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~70-~70: To join two clauses or introduce examples, consider using an em dash.
Context: ...itly specify the template file extension - Atmos will automatically use the templat...
(DASH_RULE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/config/utils.go (1)
658-700: Consider optimizing pattern matching operations.While the implementation is clean, there are a few opportunities for optimization:
- The pattern slices could be pre-allocated with a known size
- The
matchesStackFilePatternfunction could benefit from early returnsHere's a suggested optimization:
func matchesStackFilePattern(filePath, stackName string) bool { + // Early return for empty inputs + if filePath == "" || stackName == "" { + return false + } patterns := getStackFilePatterns(stackName) for _, pattern := range patterns { if strings.HasSuffix(filePath, pattern) { return true } } return false } func getConfigFilePatterns(path string, forGlobMatch bool) []string { if path == "" { return []string{} } ext := filepath.Ext(path) if ext != "" { return []string{path} } - patterns := []string{ + // Pre-allocate slice with known size + patterns := make([]string, 0, 5) + if !forGlobMatch { + // For direct file search, include the exact path without extension + patterns = append(patterns, path) + } + patterns = append(patterns, path + u.DefaultStackConfigFileExtension, path + u.DefaultStackConfigFileExtension + u.TemplateExtension, path + u.YamlTemplateExtension, path + u.YmlTemplateExtension, - } - if !forGlobMatch { - // For direct file search, include the exact path without extension - patterns = append([]string{path}, patterns...) - } + ) return patterns }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/stack_processor_utils.go(5 hunks)pkg/config/utils.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/stack_processor_utils.go
🔇 Additional comments (2)
pkg/config/utils.go (2)
28-59: LGTM! Robust pattern matching implementation.
The implementation properly handles both regular and template patterns while maintaining clear error handling. The additional comments explaining the error handling flow are particularly helpful.
702-736: Consider additional error handling cases.
The implementation efficiently uses a map for file lookups, but there are a few edge cases to consider:
Let's verify the potential edge cases:
Consider adding these safety checks:
func SearchConfigFile(configPath string, cliConfig schema.CliConfiguration) (string, error) {
+ // Validate input path
+ if configPath == "" {
+ return "", fmt.Errorf("config path cannot be empty")
+ }
+
+ // Check path length to prevent potential issues
+ if len(configPath) > 255 {
+ return "", fmt.Errorf("config path exceeds maximum length: %s", configPath)
+ }
// If path already has an extension, verify it exists
if ext := filepath.Ext(configPath); ext != "" {
what
why
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores