Ignore tmpl files to be validated #830
Conversation
ad649a3 to
15552e3
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on refining the validation process for YAML stack manifests by excluding template files from validation checks. This is achieved through modifications in the validation logic, particularly in the Changes
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/exec/validate_stacks.go (1)
137-142: Consider consolidating template file checks.The explicit skip check is redundant since these files are already excluded via
excludedPaths. Consider removing this check to avoid maintaining the same logic in two places.-// Skip template files during validation since they may contain invalid YAML before being rendered -if strings.HasSuffix(filePath, u.TemplateExtension) || - strings.HasSuffix(filePath, u.YamlTemplateExtension) || - strings.HasSuffix(filePath, u.YmlTemplateExtension) { - continue -}website/docs/core-concepts/stacks/imports.mdx (1)
72-79: LGTM! Comprehensive template file documentation.The new section provides clear and complete guidance on template file handling, including:
- Automatic detection during operations
- Validation behavior
- Extension handling
Consider using an em dash for better readability:
-Template files are skipped during `atmos validate stacks` - Atmos will find them automatically +Template files are skipped during `atmos validate stacks` — Atmos will find them automatically🧰 Tools
🪛 LanguageTool
[typographical] ~78-~78: To join two clauses or introduce examples, consider using an em dash.
Context: ...o explicitly specify template extensions - Atmos will find them automatically ::: ...(DASH_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
internal/exec/validate_stacks.go(2 hunks)pkg/config/utils.go(6 hunks)website/docs/cli/commands/validate/validate-stacks.mdx(1 hunks)website/docs/core-concepts/stacks/imports.mdx(1 hunks)website/docs/core-concepts/stacks/templates/templates.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/imports.mdx
[typographical] ~78-~78: To join two clauses or introduce examples, consider using an em dash.
Context: ...o explicitly specify template extensions - Atmos will find them automatically ::: ...
(DASH_RULE)
🔇 Additional comments (6)
internal/exec/validate_stacks.go (1)
116-122: LGTM! Comprehensive template file exclusion patterns.
The exclusion patterns correctly cover all template file extensions (.tmpl, .yaml.tmpl, .yml.tmpl).
website/docs/cli/commands/validate/validate-stacks.mdx (1)
28-29: LGTM! Clear documentation of template file handling.
The documentation clearly explains the rationale for excluding template files from validation and confirms that they are still processed during normal operations.
pkg/config/utils.go (3)
675-689: LGTM! Clean implementation of template file pattern handling.
The function now properly separates template and non-template file patterns based on the includeTemplates parameter, making it more flexible and reusable.
65-70: LGTM! Consistent template file handling across functions.
The template file skipping logic is consistently implemented in both functions using the same conditions and approach.
Also applies to: 168-173
33-34: LGTM! Clear documentation of validation behavior.
The comments clearly explain why template files are excluded during validation.
Also applies to: 136-137
website/docs/core-concepts/stacks/templates/templates.mdx (1)
23-28: LGTM! Clear and concise documentation of template file handling.
The new section effectively explains the template file validation behavior, providing users with clear understanding of:
- Which files are treated as templates (
.yaml.tmpl,.yml.tmpl,.tmpl) - How these files are handled during normal operations
- Why they are excluded from YAML validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
website/docs/core-concepts/stacks/imports.mdx (1)
72-79: Documentation looks good, but could be even clearer!The new section effectively explains the template file validation behavior. The bulleted list makes it easy to understand the key points.
Consider adding a brief example to illustrate the "unrendered placeholders" mentioned in line 73, such as:
# Example of an unrendered template that would fail YAML validation name: {{ .name }}🧰 Tools
🪛 LanguageTool
[typographical] ~78-~78: To join two clauses or introduce examples, consider using an em dash.
Context: ...o explicitly specify template extensions - Atmos will find them automatically ::: ...(DASH_RULE)
pkg/config/utils.go (1)
136-137: Consider reducing code duplication!The template file skipping logic is duplicated between
FindAllStackConfigsInPathsForStackandFindAllStackConfigsInPaths. Consider extracting this into a helper function.+func isTemplateFile(path string) bool { + return strings.HasSuffix(path, u.TemplateExtension) || + strings.HasSuffix(path, u.YamlTemplateExtension) || + strings.HasSuffix(path, u.YmlTemplateExtension) +}Then use it in both functions:
-if strings.HasSuffix(matchedFileAbsolutePath, u.TemplateExtension) || - strings.HasSuffix(matchedFileAbsolutePath, u.YamlTemplateExtension) || - strings.HasSuffix(matchedFileAbsolutePath, u.YmlTemplateExtension) { +if isTemplateFile(matchedFileAbsolutePath) {Also applies to: 168-174
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
internal/exec/validate_stacks.go(2 hunks)pkg/config/utils.go(6 hunks)website/docs/cli/commands/validate/validate-stacks.mdx(1 hunks)website/docs/core-concepts/stacks/imports.mdx(1 hunks)website/docs/core-concepts/stacks/templates/templates.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- website/docs/cli/commands/validate/validate-stacks.mdx
- website/docs/core-concepts/stacks/templates/templates.mdx
- internal/exec/validate_stacks.go
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/imports.mdx
[typographical] ~78-~78: To join two clauses or introduce examples, consider using an em dash.
Context: ...o explicitly specify template extensions - Atmos will find them automatically ::: ...
(DASH_RULE)
🔇 Additional comments (3)
pkg/config/utils.go (3)
33-34: Clean implementation of template file exclusion!
The changes effectively implement the template file exclusion during validation. The suffix checks are comprehensive and cover all template file extensions.
Also applies to: 65-71
675-689: Well-structured function update!
The addition of the includeTemplates parameter and conditional pattern inclusion is clean and maintainable. The function remains focused and follows Go best practices.
694-695: Clear and consistent implementation!
The comments and implementation consistently handle template files across different functions, making the behavior clear and maintainable.
Also applies to: 713-714
(cherry picked from commit 5402068)
15552e3 to
3fad538
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/config/utils.go (1)
694-695: Consider consolidating the template inclusion comments.The comments about including template files for normal operations are duplicated. Consider extracting this into a shared constant or documentation.
+// templateInclusionComment represents the standard comment for template inclusion +const templateInclusionComment = "Always include template files for normal operations (imports, etc.)" -// Always include template files for normal operations (imports, etc.) +// templateInclusionComment patterns := getStackFilePatterns(stackName, true)Also applies to: 713-714
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
internal/exec/validate_stacks.go(2 hunks)pkg/config/utils.go(6 hunks)website/docs/cli/commands/validate/validate-stacks.mdx(1 hunks)website/docs/core-concepts/stacks/imports.mdx(1 hunks)website/docs/core-concepts/stacks/templates/templates.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/exec/validate_stacks.go
- website/docs/cli/commands/validate/validate-stacks.mdx
- website/docs/core-concepts/stacks/templates/templates.mdx
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/imports.mdx
[typographical] ~78-~78: To join two clauses or introduce examples, consider using an em dash.
Context: ...o explicitly specify template extensions - Atmos will find them automatically ::: ...
(DASH_RULE)
🔇 Additional comments (5)
website/docs/core-concepts/stacks/imports.mdx (1)
72-79: Well-structured documentation addition!
The new section effectively communicates the template file validation behavior, with clear bullet points explaining:
- Template file detection during operations
- Validation exclusion rationale
- Automatic detection capabilities
🧰 Tools
🪛 LanguageTool
[typographical] ~78-~78: To join two clauses or introduce examples, consider using an em dash.
Context: ...o explicitly specify template extensions - Atmos will find them automatically ::: ...
(DASH_RULE)
pkg/config/utils.go (4)
675-689: Clean implementation of template file pattern handling!
The refactored getStackFilePatterns function:
- Takes a new boolean parameter to control template inclusion
- Maintains a clear separation between regular and template patterns
- Uses constants for file extensions
65-70: Consistent template file filtering implementation!
The template file filtering logic is consistently implemented across both functions using the same conditions and approach.
Also applies to: 168-173
33-34: Clear intent with descriptive comments!
The comments effectively explain why template files are excluded during validation.
Also applies to: 136-137
676-687: Verify all template file patterns are covered.
Let's ensure we're handling all possible template file patterns consistently.
✅ Verification successful
Let me check the constants and their usage to ensure we're handling all template patterns correctly.
All template file patterns are properly covered
The implementation correctly handles all template file patterns:
.tmplbase extension.yaml.tmplcompound extension.yml.tmplcompound extension
The patterns are consistently defined in pkg/utils/file_extensions.go and properly used across the codebase, particularly in:
- File extension detection in
pkg/utils/file_utils.go - Template file exclusion in validation
- Template file inclusion in imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any template files that might not match our patterns
# Search for all template files in the codebase
echo "Searching for template files..."
fd -e tmpl -e yaml.tmpl -e yml.tmpl
# Search for any potential template file references in the code
echo -e "\nSearching for template file references in code..."
rg -i "\.tmpl" --type go
Length of output: 1039
Script:
#!/bin/bash
# Check the implementation of template file handling
# First, let's see the full context of the file extensions
echo "File extension constants:"
rg -A 2 "YamlTemplateExtension" pkg/utils/file_extensions.go
# Then, check where these extensions are used
echo -e "\nUsage of template extensions:"
ast-grep --pattern 'if $_ == $_ { $$$_ }' pkg/utils/file_utils.go
# Finally, check for any other template-related patterns
echo -e "\nOther template patterns:"
rg "template" --type go -g "!*test*" -g "!*mock*"
Length of output: 28241
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/config/utils.go (3)
679-680: Consider enhancing the comment for better clarity.While the implementation is correct, the comment could be more descriptive.
-// Always include template files for normal operations (imports, etc.) +// Always include template files for normal operations (imports, stack detection, etc.) to maintain backward compatibility
698-699: Consider enhancing the comment for better clarity.While the implementation is correct, the comment could be more descriptive.
-// include template files for normal operations +// Include template files for normal operations (file search, imports) while maintaining backward compatibility
33-34: Consider enhancing the comments for better context.While the implementation is corre 2364 ct, the comments could provide more context about where and why the filtering occurs.
-// Get all patterns since filtering is done later +// Get all patterns (including templates) since specific filtering (e.g., for validation) is handled at the command levelAlso applies to: 129-130
📜 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)
661-674: LGTM! Clean implementation of template file handling.
The function now elegantly handles the inclusion/exclusion of template files through a boolean parameter, maintaining clean separation of concerns.
|
These changes were released in v1.122.0. |
what
why
references
Summary by CodeRabbit
New Features
Documentation
atmos validate stackscommand to clarify template file handling.