Support atmos docs generate feature natively #934
Conversation
TODO
|
mergify
bot
added
conflict
![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
♻️ Duplicate comments (5)
internal/exec/docs_generate.go (5)
104-104: 🛠️ Refactor suggestionConsider making 'README.yaml' filename configurable
Instead of hardcoding 'README.yaml', allow the filename to come from the configuration for greater flexibility.
117-117: 🛠️ Refactor suggestionUpdate error message for clarity
Change the error message to reflect that we're generating Terraform docs within atmos, not running an external command.
-return fmt.Errorf("failed to run terraform-docs: %w", err) +return fmt.Errorf("failed to generate terraform docs: %w", err)
156-169: 🛠️ Refactor suggestionReuse existing HTTP fetch methods
Use the existing methods (like
go-getter) for HTTP fetching to handle authentication (e.g.,GITHUB_TOKEN) and avoid duplicating code.
239-239: 🛠️ Refactor suggestionUse consistent HTTP fetch methods for templates
Consider using the same HTTP fetching mechanisms used elsewhere to handle authentication and maintain consistency.
276-295: 🛠️ Refactor suggestionLeverage existing deep merge functionality
Reuse atmos's native deep merging capabilities (possibly using
mergo) instead of implementing a customdeepMergefunction.
🧹 Nitpick comments (6)
cmd/docs_generate.go (1)
9-9: Fix typo in commentCorrect the typo: "genrates" should be "generates".
-// docsGenerateCmd genrates README.md +// docsGenerateCmd generates README.mdinternal/exec/template_utils.go (2)
226-234: Add function documentation.Consider adding a detailed function documentation comment that explains:
- The purpose of the function
- Parameter descriptions
- Return value descriptions
- Usage examples
+// ProcessTmplWithDatasourcesGomplate parses and executes Go templates with datasources using Gomplate. +// It processes the template specified by tmplName and tmplValue using the provided merged data. +// +// Parameters: +// - atmosConfig: The Atmos configuration +// - settingsSection: Settings for template processing +// - tmplName: Name of the template +// - tmplValue: The template content +// - mergedData: Data to be used in template processing +// - ignoreMissingTemplateValues: If true, missing values are replaced with empty strings +// +// Returns: +// - string: The processed template +// - error: Any error that occurred during processing func ProcessTmplWithDatasourcesGomplate(
246-259: Enhance error handling for temporary file operations.The temporary file operations could benefit from more robust cleanup and detailed error messages.
tmpfile, err := os.CreateTemp("", "gomplate-data-*.json") if err != nil { - return "", fmt.Errorf("failed to create temp data file for gomplate: %w", err) + return "", fmt.Errorf("failed to create temporary data file for gomplate (pattern: gomplate-data-*.json): %w", err) } tmpName := tmpfile.Name() -defer os.Remove(tmpName) +defer func() { + if err := os.Remove(tmpName); err != nil { + // Log the error but don't fail the operation + u.LogWarning(atmosConfig, fmt.Sprintf("failed to remove temporary file %s: %v", tmpName, err)) + } +}()pkg/schema/schema.go (2)
195-204: Add field documentation for TerraformDocsSettings.Consider adding documentation comments for each field to explain their purpose and valid values.
type TerraformDocsSettings struct { + // Enabled determines if Terraform documentation generation is active Enabled bool `yaml:"enabled,omitempty" json:"enabled,omitempty" mapstructure:"enabled,omitempty"` + // Format specifies the output format (e.g., "markdown", "asciidoc") Format string `yaml:"format,omitempty" json:"format,omitempty" mapstructure:"format,omitempty"` + // ShowProviders controls whether provider documentation is included ShowProviders bool `yaml:"show_providers,omitempty" json:"show_providers,omitempty" mapstructure:"show_providers,omitempty"` + // ShowInputs controls whether input variable documentation is included ShowInputs bool `yaml:"show_inputs,omitempty" json:"show_inputs,omitempty" mapstructure:"show_inputs,omitempty"` + // ShowOutputs controls whether output variable documentation is included ShowOutputs bool `yaml:"show_outputs,omitempty" json:"show_outputs,omitempty" mapstructure:"show_outputs,omitempty"` + // SortBy defines the sorting criteria for documentation elements (e.g., "name", "required") SortBy string `yaml:"sort_by,omitempty" json:"sort_by,omitempty" mapstructure:"sort_by,omitempty"` + // HideEmpty determines if empty sections should be omitted from the output HideEmpty bool `yaml:"hide_empty,omitempty" json:"hide_empty,omitempty" mapstructure:"hide_empty,omitempty"` + // IndentLevel specifies the number of spaces to use for indentation IndentLevel int `yaml:"indent_level,omitempty" json:"indent_level,omitempty" mapstructure:"indent_level,omitempty"` }
206-211: Enhance DocsGenerate struct with documentation and validation.Consider adding field documentation and validation tags to ensure proper usage.
type DocsGenerate struct { + // Input is a list of paths to input files (e.g., README.yaml) + // At least one input file must be specified Input []string `yaml:"input,omitempty" json:"input,omitempty" mapstructure:"input"` + // Template is a list of template files to be used for generation + // At least one template file must be specified Template []string `yaml:"template,omitempty" json:"template,omitempty" mapstructure:"template"` + // Output specifies the path where the generated documentation will be written Output string `yaml:"output,omitempty" json:"output,omitempty" mapstructure:"output"` + // Terraform contains settings specific to Terraform documentation generation Terraform TerraformDocsSettings `yaml:"terraform,omitempty" json:"terraform,omitempty" mapstructure:"terraform"` }atmos.yaml (1)
398-404: Consider making paths more configurable.The hardcoded paths could be made more flexible by supporting:
- Environment variable substitution
- Relative path resolution
- Multiple file patterns
input: - - "./README.yaml" + - "${ATMOS_README_INPUT:-./README.yaml}" + - "**/*.yaml" # Support recursive search template: - - "./README.md.gotmpl" + - "${ATMOS_README_TEMPLATE:-./README.md.gotmpl}" + - "https://raw.githubusercontent.com/org/repo/main/templates/*.gotmpl" output: "README.md"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
atmos.yaml(1 hunks)cmd/docs_generate.go(1 hunks)go.mod(8 hunks)internal/exec/docs_generate.go(1 hunks)internal/exec/file_utils.go(2 hunks)internal/exec/template_utils.go(8 hunks)pkg/schema/schema.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
internal/exec/file_utils.go
[failure] 7-7:
url redeclared in this block
[failure] 7-7:
"net/url" imported and not used
[failure] 12-12:
filepath redeclared in this block
[failure] 9-9:
other declaration of filepath
[failure] 12-12:
"path/filepath" imported and not used
[failure] 13-13:
runtime redeclared in this block
[failure] 10-10:
other declaration of runtime
[failure] 13-13:
"runtime" imported and not used
[failure] 14-14:
strings redeclared in this block
🪛 GitHub Check: Build (windows-latest, windows)
internal/exec/file_utils.go
[failure] 7-7:
url redeclared in this block
[failure] 7-7:
"net/url" imported and not used
[failure] 12-12:
filepath redeclared in this block
[failure] 9-9:
other declaration of filepath
[failure] 12-12:
"path/filepath" imported and not used
[failure] 13-13:
runtime redeclared in this block
[failure] 10-10:
other declaration of runtime
[failure] 13-13:
"runtime" imported and not used
[failure] 14-14:
strings redeclared in this block
🪛 GitHub Check: Build (ubuntu-latest, linux)
internal/exec/file_utils.go
[failure] 7-7:
url redeclared in this block
[failure] 7-7:
"net/url" imported and not used
[failure] 12-12:
filepath redeclared in this block
[failure] 9-9:
other declaration of filepath
[failure] 12-12:
"path/filepath" imported and not used
[failure] 13-13:
runtime redeclared in this block
[failure] 10-10:
other declaration of runtime
[failure] 13-13:
"runtime" imported and not used
[failure] 14-14:
strings redeclared in this block
🪛 GitHub Actions: Tests
internal/exec/file_utils.go
[error] 7-7: url redeclared in this block
go.mod
[error] 53-69: Module dependencies are not properly ordered. Run 'go mod tidy' to fix the ordering and commit the changes.
🪛 yamllint (1.35.1)
atmos.yaml
[error] 413-413: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
cmd/docs_generate.go (1)
23-32: Enhance readability of command examplesWrap command examples in code fences for better formatting in the help output.
Examples: - Generate a README.md in the current path: + ``` atmos docs generate + ``` - Generate a README.md for the VPC component: + ``` atmos docs generate components/terraform/vpc + ``` - Generate all README.md (recursively searches for README.yaml to rebuild docs): + ``` atmos docs generate --all + ```atmos.yaml (2)
397-398: Address TODO comment regarding README.yaml location logic.The TODO comment indicates missing functionality for finding README.yaml files. Consider implementing this before merging.
Would you like me to help implement the logic for finding README.yaml files based on the repository owner/organization structure?
400-402: Address TODO comment regarding remote templates.The TODO comment suggests that remote template support is planned. Consider implementing this capability now.
Would you like me to help implement support for remote template URLs?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/file_utils.go (2)
101-124: Strong implementation with clear documentation! 💪The function effectively handles path conversion for both Windows and Linux systems, with excellent documentation and examples.
Consider optimizing the Windows path handling.
The Windows path handling could be simplified using an unconditional
TrimPrefix:- if strings.HasPrefix(pathSlashed, "/") { - pathSlashed = strings.TrimPrefix(pathSlashed, "/") - } + pathSlashed = strings.TrimPrefix(pathSlashed, "/")🧰 Tools
🪛 golangci-lint (1.62.2)
112-112: S1017: should replace this
ifstatement with an unconditionalstrings.TrimPrefix(gosimple)
126-142: Consider making the drive letter check more explicit! 🎯The drive letter check on line 136 could be more explicit about its purpose:
- if strings.HasPrefix(u.Path, "/") && len(u.Path) > 2 && u.Path[2] == ':' { + // Check if path starts with a drive letter (e.g., "/C:/...") + if strings.HasPrefix(u.Path, "/") && len(u.Path) > 2 && + ((u.Path[1] >= 'A' && u.Path[1] <= 'Z') || (u.Path[1] >= 'a' && u.Path[1] <= 'z')) && + u.Path[2] == ':' {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(9 hunks)internal/exec/file_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/exec/file_utils.go
112-112: S1017: should replace this if statement with an unconditional strings.TrimPrefix
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/exec/docs_generate.go (3)
83-143: Potential concurrency caveat withos.Chdir.This method’s reliance on
os.Chdirfor each subdirectory works well for a single-threaded flow. If you plan to expand concurrency or parallel execution in the future,os.Chdirwould pose a thread safety concern.
265-273: Error logging approach for remote or local file merges.This function streams the result back to the caller for merging. It remains concise. Do note that each fetch is performed serially; if performance becomes an issue with many inputs, consider parallel fetch with safe merges later.
317-343: Optional: Make the 10-minute timeout configurable.The hard-coded
10 * time.Minuteis a sensible default but might benefit from user configurability in large or constrained networking environments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/docs_generate.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/docs_generate.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (10)
internal/exec/docs_generate.go (10)
36-81: Solid top-level command logic.This function implements the
docs generatecommand end-to-end, neatly handling CLI flags and re-initializing config once. The approach of loading configurations only once and using them for subsequent directory scans aligns well with the PR objectives.
145-182: Reusable approach for single directory generation.Re-initializing local config if
atmos.yamlis found intargetDiris a straightforward fallback mechanism. The method is well structured for single-usage scenarios, matching the design intent for local overrides.
184-263: Graceful handling of YAML input errors.Skipping invalid YAML inputs and proceeding with the remaining sources is consistent with our retrieved learnings about not failing fast on partial errors. This design effectively captures all possible merges while keeping logs.
275-281: Simple file reading logic.The logic is straightforward, and error handling is done properly.
283-289: Efficient unmarshal withyaml.Unmarshal.This function is short, direct, and effective.
291-315: Terraform Docs integration.Loading the Terraform module and generating Markdown output is done neatly. The configuration flags are minimal, but flexible for typical usage.
317-343: Inconsistency in named return parameter.The function signature uses
(localPath string, temDir string, err error), but the code referencestempDir. This mismatch can confuse maintainers.Apply this diff to unify naming:
-func downloadSource( - atmosConfig schema.AtmosConfiguration, - pathOrURL string, - baseDir string, -) (localPath string, temDir string, err error) { +func downloadSource( + atmosConfig schema.AtmosConfiguration, + pathOrURL string, + baseDir string, +) (localPath string, tempDir string, err error) {
345-376: Restrictive single-file check.This function returns an error if more than one file is found, which keeps usage predictable but might restrict future multi-file template scenarios. Consider allowing multiple files if your use case evolves.
378-387: Minimal remote-check logic looks adequate.The prefix-based approach should suffice for typical protocols. Future expansions may require a more robust pattern for additional VCS or custom schemes.
389-399: Simple existence check.This helper method straightforwardly checks for file presence. Implementation is direct and error handling is appropriate.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/exec/template_utils.go (2)
274-313: Enhance error messages with more context.While the error handling is thorough, the error messages could be more specific about what data failed to process.
Consider this improvement:
- return "", fmt.Errorf("failed to marshal merged data to JSON: %w", err) + return "", fmt.Errorf("failed to marshal template data for '%s' to JSON: %w", tmplName, err)
355-389: Consider adding timeout for template rendering.While the implementation is solid, adding a timeout context for the rendering operation would prevent potential hangs with complex templates.
Consider this improvement:
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() - if err := renderer.RenderTemplates(context.Background(), []gomplate.Template{tpl}); err != nil { + if err := renderer.RenderTemplates(ctx, []gomplate.Template{tpl}); err != nil {pkg/schema/schema.go (1)
197-206: Consider adding field documentation.The struct is well-designed, but adding documentation comments for each field would improve maintainability. For example:
type TerraformDocsSettings struct { + // Enabled controls whether Terraform documentation generation is active Enabled bool `yaml:"enabled,omitempty" json:"enabled,omitempty" mapstructure:"enabled,omitempty"` + // Format specifies the output format for the documentation (e.g., markdown) Format string `yaml:"format,omitempty" json:"format,omitempty" mapstructure:"format,omitempty"` // ... add similar comments for other fields }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/docs_generate.go(1 hunks)internal/exec/template_utils.go(2 hunks)pkg/schema/schema.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/docs_generate.go
🧰 Additional context used
📓 Learnings (1)
internal/exec/template_utils.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#934
File: internal/exec/template_utils.go:268-271
Timestamp: 2025-01-25T03:49:03.951Z
Learning: The `ProcessTmplWithDatasourcesGomplate` function in `internal/exec/template_utils.go` is used for documentation generation purposes, where simple environment variable handling is acceptable and thread-safety concerns are not critical.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/template_utils.go (2)
258-273: LGTM! Function signature and environment variable handling are appropriate for docs generation.The function parameters are well-structured, and while setting global environment variables isn't typically recommended, it's acceptable here for documentation generation purposes where thread-safety isn't critical.
315-354: LGTM! Well-documented outer data handling with cross-platform support.The implementation shows excellent attention to detail:
- Clear documentation of the two-layer data structure
- Proper Windows path handling for cross-platform compatibility
- Thorough error handling with cleanup
pkg/schema/schema.go (2)
90-93: Well-documented deprecation notices!The deprecation comments clearly indicate where these settings have been moved to, which helps with migration.
208-213: Consider adding input validation.The struct looks good, but consider adding validation for:
- Input paths existence
- Template file existence and format
- Output path writability
Run this script to check if validation is implemented elsewhere:
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/exec/file_utils.go (2)
101-124: Strengthen the implementation with input validation and clearer examples.The function handles path conversion well, but could be enhanced:
- Add input validation to check for empty paths
- Expand the comments with concrete examples for both Windows and Linux paths
- Simplify the Windows path handling
// toFileURL converts a local filesystem path into a "file://" URL in a way // that won't confuse Gomplate on Windows or Linux. // -// On Windows, e.g. localPath = "D:\Temp\foo.json" => "file://D:/Temp/foo.json" -// On Linux, e.g. localPath = "/tmp/foo.json" => "file:///tmp/foo.json" +// Examples: +// Windows: +// Input: "D:\Temp\foo.json" Output: "file://D:/Temp/foo.json" +// Input: "/D:/Temp/foo.json" Output: "file://D:/Temp/foo.json" +// Input: "C:\Program Files\foo" Output: "file://C:/Program Files/foo" +// Linux: +// Input: "/tmp/foo.json" Output: "file:///tmp/foo.json" +// Input: "tmp/foo.json" Output: "file:///tmp/foo.json" func toFileScheme(localPath string) (string, error) { + if localPath == "" { + return "", fmt.Errorf("localPath cannot be empty") + } pathSlashed := filepath.ToSlash(localPath) if runtime.GOOS == "windows" { - // If pathSlashed is "/D:/Temp/foo.json", remove the leading slash => "D:/Temp/foo.json" - // Then prepend "file://" - if strings.HasPrefix(pathSlashed, "/") { - pathSlashed = strings.TrimPrefix(pathSlashed, "/") // e.g. "D:/Temp/foo.json" - } + // Always trim leading slash for Windows paths + pathSlashed = strings.TrimPrefix(pathSlashed, "/") return "file://" + pathSlashed, nil // e.g. "file://D:/Temp/foo.json" }🧰 Tools
🪛 golangci-lint (1.62.2)
112-112: S1017: should replace this
ifstatement with an unconditionalstrings.TrimPrefix(gosimple)
126-142: Enhance error handling and documentation.The function handles URL parsing well, but could be improved:
- Add examples in comments to clarify the transformations
- Add more specific error messages for different failure cases
- Consider adding validation for the resulting path
+// fixWindowsFileScheme fixes Windows file:// URLs to handle drive letters correctly. +// +// Examples: +// Input: "file://D:/path/file.txt" Output: "file:///D:/path/file.txt" +// Input: "file:///D:/path/file.txt" Output: "file:///D:/path/file.txt" +// Input: "file://localhost/D:/path.txt" Output: "file:///D:/path.txt" func fixWindowsFileScheme(rawURL string) (*url.URL, error) { u, err := url.Parse(rawURL) if err != nil { - return nil, fmt.Errorf("failed to parse URL %q: %w", rawURL, err) + return nil, fmt.Errorf("invalid URL format %q: %w", rawURL, err) } + + if u.Scheme != "" && u.Scheme != "file" { + return nil, fmt.Errorf("expected 'file' scheme, got %q", u.Scheme) + } if runtime.GOOS == "windows" && u.Scheme == "file" { if len(u.Host) > 0 { + // Move host to path for Windows UNC paths u.Path = u.Host + u.Path u.Host = "" } + // Handle drive letter paths (e.g., /D:/path/file.txt -> D:/path/file.txt) if strings.HasPrefix(u.Path, "/") && len(u.Path) > 2 && u.Path[2] == ':' { u.Path = strings.TrimPrefix(u.Path, "/") } } + // Validate the resulting path + if _, err := filepath.Abs(u.Path); err != nil { + return nil, fmt.Errorf("invalid path after URL fix: %w", err) + } return u, nil }internal/exec/template_utils.go (2)
268-273: Consider adding error handling for environment variable operations.While the current implementation is acceptable for documentation generation purposes (as per previous discussions), it would be more robust to handle potential errors from environment variable operations.
if ignoreMissingTemplateValues { - os.Setenv("GOMPLATE_MISSINGKEY", "default") - defer os.Unsetenv("GOMPLATE_MISSINGKEY") + if err := os.Setenv("GOMPLATE_MISSINGKEY", "default"); err != nil { + return "", fmt.Errorf("failed to set GOMPLATE_MISSINGKEY: %w", err) + } + defer func() { + if err := os.Unsetenv("GOMPLATE_MISSINGKEY"); err != nil { + // Log error since we can't return it from defer + fmt.Fprintf(os.Stderr, "failed to unset GOMPLATE_MISSINGKEY: %v\n", err) + } + }() }
303-312: Consider consolidating URL transformation functions.The code makes two separate calls to URL transformation functions. Consider consolidating these into a single helper function for better maintainability.
+func toWindowsCompatibleFileURL(path string) (string, error) { + fileURL, err := toFileScheme(path) + if err != nil { + return "", fmt.Errorf("failed to convert path to file URL: %w", err) + } + return fixWindowsFileScheme(fileURL) +} // In the main function: -fileURL, err := toFileScheme(tmpName) -if err != nil { - return "", fmt.Errorf("failed to convert temp file path to file URL: %w", err) -} -finalFileUrl, err := fixWindowsFileScheme(fileURL) +finalFileUrl, err := toWindowsCompatibleFileURL(tmpName)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/file_utils.go(1 hunks)internal/exec/template_utils.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/template_utils.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#934
File: internal/exec/template_utils.go:268-271
Timestamp: 2025-01-25T03:49:03.951Z
Learning: The `ProcessTmplWithDatasourcesGomplate` function in `internal/exec/template_utils.go` is used for documentation generation purposes, where simple environment variable handling is acceptable and thread-safety concerns are not critical.
🪛 golangci-lint (1.62.2)
internal/exec/file_utils.go
112-112: S1017: should replace this if statement with an unconditional strings.TrimPrefix
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/file_utils.go (1)
100-142: Well-structured implementation for cross-platform URL handling!These utility functions provide robust support for the new
atmos docs generatefeature, ensuring consistent file URL handling across Windows and Linux. The implementation aligns well with the PR objectives for documentation generation.🧰 Tools
🪛 golangci-lint (1.62.2)
112-112: S1017: should replace this
ifstatement with an unconditionalstrings.TrimPrefix(gosimple)
internal/exec/template_utils.go (4)
258-266: LGTM! Clear and well-structured function signature.The function parameters are well-organized and provide all necessary inputs for template processing.
274-301: LGTM! Well-structured temporary file handling.The code properly:
- Creates temporary files with meaningful prefixes
- Defers cleanup
- Handles all potential error cases
- Includes clear comments explaining the purpose
373-388: LGTM! Clean template rendering implementation.The rendering logic is well-structured with proper error handling and context management.
361-371: Consider extending template functions.The
Funcsfield ingomplate.Optionsis currently empty. Consider adding custom functions that might be useful for documentation generation.Let's check if there are any existing template functions we should include:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/test-cases/docs-generate.yaml (5)
1-1: Consider using an absolute schema path.The relative schema path
schema.jsonmight not resolve correctly in different environments. Consider using an absolute path or moving the schema to a well-known location.-# yaml-language-server: $schema=schema.json +# yaml-language-server: $schema=https://raw.githubusercontent.com/cloudposse/atmos/master/tests/test-cases/schema.json
3-22: Strengthen test assertions for basic docs generation.The test case verifies basic functionality but could be more robust. Consider adding:
- File permission checks for the generated README.md
- Content structure validation beyond just the "Atmos" string
- Timestamp checks to verify file freshness
file_contains: "README.md": - - "Atmos" + - "Atmos" + - "## Table of Contents" + - "## Overview" + file_permissions: + "README.md": "0644" + file_age: + "README.md": "<1m"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 14-14: trailing spaces
(trailing-spaces)
[error] 21-21: trailing spaces
(trailing-spaces)
43-43: Fix typo in test name.There's a spelling error in the test name.
- - name: "atmos docs generate with path as agrument" + - name: "atmos docs generate with path as argument"
79-82: Add checks for additional Terraform doc sections.The test verifies basic Terraform doc sections but misses some important ones.
file_contains: "README.md": - "## Requirements" - "## Providers" - - "## Inputs" + - "## Inputs" + - "## Outputs" + - "## Resources" + - "## Modules" + - "## Example"🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 82-82: trailing spaces
(trailing-spaces)
14-14: Remove trailing spaces.Several lines contain trailing spaces. While this doesn't affect functionality, it's good practice to remove them for consistency.
Affected lines: 14, 21, 41, 61, 82
Also applies to: 21-21, 41-41, 61-61, 82-82
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 14-14: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(9 hunks)pkg/schema/schema.go(2 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/test-cases/docs-generate.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- pkg/schema/schema.go
- go.mod
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/docs-generate.yaml
[error] 14-14: trailing spaces
(trailing-spaces)
[error] 21-21: trailing spaces
(trailing-spaces)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 61-61: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
| - name: "atmos docs generate with subdirectories" | ||
| enabled: true | ||
| description: "Ensure docs are generated for subdirectories." | ||
| workdir: "../" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use more specific workdir path.
The "../" workdir is fragile and could break if test directory structure changes. Consider using a more specific path relative to the test root.
- workdir: "../"
+ workdir: "fixtures/scenarios/"Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/docs_generate.go (1)
243-247: Move default values to the schema model.Consider moving the default output filename to the schema definition instead of computing it dynamically. This would make the defaults more discoverable and maintainable.
Example schema change:
type DocsGenerate struct { Output string `yaml:"output" default:"README.md"` // ... other fields }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/docs_generate.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/docs_generate.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#934
File: internal/exec/docs_generate.go:98-101
Timestamp: 2025-01-25T04:01:58.095Z
Learning: In the `generateSingleReadme` function of the docs generation feature (internal/exec/docs_generate.go), errors from `fetchAndParseYAML` should be logged and skipped rather than causing early returns. This is by design to process all inputs and collect all errors, instead of failing fast on the first error.
🪛 golangci-lint (1.62.2)
internal/exec/docs_generate.go
111-111: Error return value of os.Chdir is not checked
(errcheck)
157-157: Error return value of os.Chdir is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/docs_generate.go (2)
184-192: Well-structured error handling for YAML inputs!The implementation correctly processes all inputs while logging errors, allowing the generation to continue even if some inputs fail. This aligns with the design goal of collecting all errors instead of failing fast.
370-379: Clear and documented URL validation approach!The implementation provides basic remote path detection while maintaining simplicity, as intended by design. The code is well-documented with clear reasoning about the chosen approach.
| if err != nil { | ||
| return err | ||
| } | ||
| defer os.Chdir(oldDir) // ensure we go back no matter what |
There was a problem hiding this comment.
Add error handling for deferred directory changes.
The deferred os.Chdir calls should handle potential errors to ensure the process maintains the correct working directory.
Apply this diff to add error handling:
- defer os.Chdir(oldDir)
+ defer func() {
+ if err := os.Chdir(oldDir); err != nil {
+ u.LogError(atmosConfig, fmt.Sprintf("Failed to restore working directory: %v", err))
+ }
+ }()Also applies to: 157-157
🧰 Tools
🪛 golangci-lint (1.62.2)
111-111: Error return value of os.Chdir is not checked
(errcheck)
what
This PR implements a native atmos command to generate readmes like this
atmos docs generateto generate readmes for the current folder,atmos docs generate --allto generate readmes for the current and all subfolders.Terraform docs generation is also supported. To enable it, set respective parameters in the docs section of atmos.yaml.
E.g. this one generates both the readmes and components/variables descriptions based on the output of terraform-doc.
atmos docs generate examples\quick-start-advanced\components\terraform\vpcTesting
Refer to tests\test-cases\docs-generate.yaml
--- PASS: TestCLICommands (0.99s) --- PASS: TestCLICommands/atmos_docs_generate_in_current_directory (0.18s) --- PASS: TestCLICommands/atmos_docs_generate_with_subdirectories (0.44s) --- PASS: TestCLICommands/atmos_docs_generate_with_path_as_agrument (0.18s) --- PASS: TestCLICommands/atmos_docs_generate_for_terraform_docs (0.19s)why
Have an option to run a command natively from within atmos (not using builld-harness repo)
references
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
atmos docs generateREADME.mdfiles from YAML configurationsgeneratecommandImprovements
Dependencies
The release introduces a powerful new documentation generation feature for Atmos, allowing users to automatically create and customize README files for their components and stacks.