package manager for vendor pull #768
Conversation
…uce package types
… package handling
We need a fallback for headless terminals. This is probably handled by charmbracelet as well. We might just not be using it. |
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces updates focused on dependency management in 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: 6
🧹 Outside diff range and nitpick comments (9)
internal/exec/vendor_model.go (1)
148-148: Add missing space in completion messageThere's a missing space after the period in the string "Vendored %d components.Failed to vendor %d components.\n", which causes the message to read incorrectly. Adding a space will improve readability.
Apply this diff to fix the formatting:
return doneStyle.Render(fmt.Sprintf("Vendored %d components.Failed to vendor %d components.\n", n-m.failedPkg, m.failedPkg)) +return doneStyle.Render(fmt.Sprintf("Vendored %d components. Failed to vendor %d components.\n", n-m.failedPkg, m.failedPkg))internal/exec/vendor_model_component.go (5)
23-34: ExportpkgComponentVendorstructIf there's a possibility of using
pkgComponentVendoroutside this package in the future, consider exporting it by capitalizing the struct name. Adding comments to structs and fields will also enhance code readability.
135-137: Add missing space in status messageThere's a missing space after the period in the status message. This will improve the readability of the output.
-return doneStyle.Render(fmt.Sprintf("Vendored %d components.Failed to vendor %d components.\n", n-m.failedPkg, m.failedPkg)) +return doneStyle.Render(fmt.Sprintf("Vendored %d components. Failed to vendor %d components.\n", n-m.failedPkg, m.failedPkg))
166-199: Refactor to eliminate duplicate code indownloadComponentAndInstallThe function
downloadComponentAndInstallcontains similar code blocks for handling components and mixins. Refactoring can reduce duplication and simplify maintenance.Here's a possible refactor:
func downloadComponentAndInstall(p pkgComponentVendor, dryRun bool, cliConfig schema.CliConfiguration) tea.Cmd { return func() tea.Msg { if dryRun { time.Sleep(100 * time.Millisecond) return installedPkgMsg{ err: nil, name: p.name, } } - if p.IsComponent { - err := installComponent(p, cliConfig) - if err != nil { - return installedPkgMsg{ - err: err, - name: p.name, - } - } - return installedPkgMsg{ - err: nil, - name: p.name, - } - } - if p.IsMixins { - err := installMixin(p, cliConfig) - if err != nil { - return installedPkgMsg{ - err: err, - name: p.name, - } - } - return installedPkgMsg{ - err: nil, - name: p.name, - } - } + var err error + if p.IsComponent { + err = installComponent(p, cliConfig) + } else if p.IsMixins { + err = installMixin(p, cliConfig) + } else { + err = fmt.Errorf("unknown install operation") + } return installedPkgMsg{ err: err, name: p.name, } } }
258-259: Include package type in error messageAdding the package type to the error message will help in debugging unknown package types.
-return fmt.Errorf("unknown package type") +return fmt.Errorf("unknown package type: %v", p.pkgType)
297-298: Include package type in error messageSimilarly, in
installMixin, include the package type in the error message for better clarity.-return fmt.Errorf("unknown package type") +return fmt.Errorf("unknown package type: %v", p.pkgType)internal/exec/vendor_component_utils.go (3)
96-105: ImplementExecuteStackVendorInternalfunctionThe
ExecuteStackVendorInternalfunction currently returns a "not supported yet" error. Consider implementing the necessary logic or providing a more detailed message.Would you like assistance in drafting the implementation or creating a GitHub issue to track this task?
343-343: Wrap error with%wfor proper error handlingTo enable error unwrapping, replace
%vwith%wwhen formatting errors.Apply this diff:
- return fmt.Errorf("error initializing model: %v", err) + return fmt.Errorf("error initializing model: %w", err)
341-345: Handle errors from the TUI runEnsure that errors from
tea.NewProgram(model).Run()are properly handled to maintain robustness.Apply this diff:
if _, err := tea.NewProgram(model).Run(); err != nil { - return fmt.Errorf("running download error: %w", err) + return fmt.Errorf("error running TUI program: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(1 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_model_component.go(1 hunks)internal/exec/vendor_utils.go(5 hunks)
🔇 Additional comments (10)
go.mod (1)
96-96: LGTM! Clean dependency addition for TUI animations.
The addition of github.com/charmbracelet/harmonica aligns perfectly with implementing an improved vendor pull interface using Bubble Tea. This indirect dependency will provide smooth animation capabilities for the TUI.
Let's verify the harmonica package usage in the codebase:
internal/exec/vendor_utils.go (8)
10-10: Importing Bubble Tea library for TUI functionality
The addition of the tea import integrates the Bubble Tea library, enabling the new TUI enhancements.
178-179: Calling logInitialMessage to enhance user feedback
Incorporating logInitialMessage improves the user experience by logging initial processing information.
Line range hint 250-309: Refactoring source processing into modular functions
The introduction of shouldSkipSource and validateSourceFields enhances code readability and maintainability by encapsulating specific logic.
271-280: Clarifying source type determination logic
The determineSourceType function cleanly separates the logic for source type determination, improving code clarity.
311-321: Implementing TUI for package processing
Introducing the TUI enhances the user interface during package processing with Bubble Tea.
372-379: Defining logInitialMessage function
Centralizing initial logging supports better code organization and reduces redundancy.
380-392: Duplicate issue: Modifying a value receiver in validateSourceFields
As mentioned earlier, modifying s.File won't affect the original struct since s is passed by value.
435-491: Generating skip function for file copying
The generateSkipFunction effectively handles inclusion and exclusion patterns, enhancing modularity and reusability.
internal/exec/vendor_component_utils.go (1)
15-15: Bubble Tea import added for TUI integration
The addition of the Bubble Tea package is appropriate for implementing TUI features, aligning with the PR objectives.
| if strings.HasSuffix(src, ".git") { | ||
| return true, nil |
There was a problem hiding this comment.
Correct .git directory exclusion logic
Using strings.HasSuffix(src, ".git") may incorrectly exclude files or directories ending with .git. To specifically exclude the .git directory, use filepath.Base(src) == ".git".
Apply this diff:
- if strings.HasSuffix(src, ".git") {
+ if filepath.Base(src) == ".git" {
return true, nil
}📝 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.
| if strings.HasSuffix(src, ".git") { | |
| return true, nil | |
| if filepath.Base(src) == ".git" { | |
| return true, nil |
There was a problem hiding this comment.
@aknysh what do you think about this?
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!
There was a problem hiding this comment.
I think filepath.Base(src) == ".git" is better at detecting if the file exists and we should use it
@haitham911 please update the code and test
|
Running 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
examples/demo-component-versions/atmos.yaml (1)
28-28: Fix indentation for consistencyThe command step should maintain consistent indentation with other YAML elements.
Apply this diff to fix the indentation:
- atmos vendor pull --everything + atmos vendor pull --everything🧰 Tools
🪛 yamllint
[warning] 28-28: wrong indentation: expected 4 but found 2
(indentation)
examples/demo-vendoring/atmos.yaml (1)
30-30: Consider adding documentation for the --everything flagThe addition of
--everythingflag aligns with the PR objectives to enhance the vendor pull functionality. However, since this is a demo file, it would be helpful to add comments explaining:
- The purpose of the
--everythingflag- How it differs from the default behavior
- Any considerations for non-TTY environments
Consider adding comments above the command:
commands: - name: "test" description: "Run all tests" steps: + # Uses the interactive package manager to pull all vendor components + # For non-TTY environments, redirect output: atmos vendor pull --everything > pull.log - atmos vendor pull --everything
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
examples/demo-component-versions/atmos.yaml(1 hunks)examples/demo-vendoring/atmos.yaml(1 hunks)go.mod(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 yamllint
examples/demo-component-versions/atmos.yaml
[warning] 28-28: wrong indentation: expected 4 but found 2
(indentation)
🔇 Additional comments (2)
examples/demo-component-versions/atmos.yaml (1)
28-28: Verify CI/CD compatibility with the new --everything flag
The addition of the --everything flag aligns with the new interactive package manager interface. However, since this is in the test command, we should ensure it works correctly in non-interactive CI/CD environments.
Consider adding a comment above the command to document the expected behavior in non-TTY environments, as this could affect automated testing scenarios.
🧰 Tools
🪛 yamllint
[warning] 28-28: wrong indentation: expected 4 but found 2
(indentation)
examples/demo-vendoring/atmos.yaml (1)
30-30: Verify the command's behavior in non-TTY environments
Given the concerns about TTY handling mentioned in the PR comments, we should verify that this example works in headless environments.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
internal/exec/vendor_model_component.go (3)
22-33: Add documentation and fix naming inconsistency.The struct is well-organized, but could benefit from:
- Adding documentation comments for the struct and its fields
- Renaming
IsMixinstoIsMixinfor consistency withIsComponent+// pkgComponentVendor represents a vendor component package with its metadata and configuration type pkgComponentVendor struct { + // uri is the source location of the package uri string // ... add comments for other fields - IsMixins bool + IsMixin bool mixinFilename string }
67-111: Extract common installation logic.The download and installation logic contains duplicate patterns between component and mixin handling. Consider extracting common functionality into a shared helper function.
+func handleInstallation(p *pkgComponentVendor, installer func(*pkgComponentVendor, schema.CliConfiguration) error, cliConfig schema.CliConfiguration) installedPkgMsg { + err := installer(p, cliConfig) + return installedPkgMsg{ + err: err, + name: p.name, + } +} func downloadComponentAndInstall(p *pkgComponentVendor, dryRun bool, cliConfig schema.CliConfiguration) tea.Cmd { return func() tea.Msg { if dryRun { // ... dry run code ... } if p.IsComponent { - err := installComponent(p, cliConfig) - if err != nil { - return installedPkgMsg{ - err: err, - name: p.name, - } - } - return installedPkgMsg{ - err: nil, - name: p.name, - } + return handleInstallation(p, installComponent, cliConfig) } if p.IsMixins { - err := installMixin(p, cliConfig) - // ... similar code ... + return handleInstallation(p, installMixin, cliConfig) } // ... rest of the code ... } }
185-248: Enhance error messages in installMixin.The error messages could be more descriptive to help with debugging.
- return fmt.Errorf("local mixin installation not implemented") + return fmt.Errorf("local mixin installation not implemented for URI: %s", p.uri)internal/exec/vendor_model.go (2)
23-58: Add documentation for types and structs.Consider adding documentation comments for the types and structs to improve code maintainability. This will help other developers understand the purpose and usage of each field.
+// pkgType represents the type of package being vendored type pkgType int +// Package type constants const ( pkgTypeRemote pkgType = iota pkgTypeOci pkgTypeLocal ) +// pkgVendor represents a vendor package with its metadata type pkgVendor struct { name string version string atmosPackage *pkgAtmosVendor componentPackage *pkgComponentVendor }
194-196: Enhance error reporting clarity.The current message could be clearer about successful vs. failed components. Consider adding more context to help users understand the outcome.
-return doneStyle.Render(fmt.Sprintf("Vendored %d components. Failed to vendor %d components.\n", n-m.failedPkg, m.failedPkg)) +return doneStyle.Render(fmt.Sprintf("Summary: %d components successfully vendored, %d components failed.\nCheck the logs above for details on failed components.\n", n-m.failedPkg, m.failedPkg))internal/exec/vendor_component_utils.go (1)
98-105: TODO comment needs implementation timelineThe
ExecuteStackVendorInternalfunction is currently a placeholder. Consider adding a timeline for implementation or tracking this in an issue.Would you like me to create a GitHub issue to track the implementation of the stack vendor functionality?
internal/exec/vendor_utils.go (2)
325-341: Enhance error messages in TUI initialization.The error messages could be more descriptive to help with troubleshooting:
- return fmt.Errorf("error initializing model: %v", err) + return fmt.Errorf("failed to initialize TUI model: %v (check terminal capabilities)", err) - return fmt.Errorf("running atmos vendor internal download error: %w", err) + return fmt.Errorf("TUI download process failed: %w", err)
402-414: Add version format validation in validateSourceFields.Consider adding version format validation to ensure semantic versioning compliance when a version is specified.
func validateSourceFields(s *schema.AtmosVendorSource, vendorConfigFileName string) error { + if s.Version != "" { + if !isValidSemver(s.Version) { + return fmt.Errorf("invalid version format '%s' in vendor config file '%s'", s.Version, s.File) + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_model_component.go(1 hunks)internal/exec/vendor_utils.go(8 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/vendor_component_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#768
File: internal/exec/vendor_component_utils.go:354-360
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the vendoring process, a TTY can exist without being interactive. If the process does not prompt the user, we should not require interactive mode to display the TUI. The `CheckTTYSupport` function should check TTY support on stdout rather than stdin.
🔇 Additional comments (6)
internal/exec/vendor_model_component.go (2)
3-20: LGTM! Well-organized imports.
The imports are clean and appropriate for the task. The Bubble Tea framework imports align well with the PR objective of creating an interactive terminal UI.
35-65: LGTM! Solid initialization logic.
The model initialization is well-implemented with appropriate progress bar configuration and empty package handling.
internal/exec/vendor_model.go (1)
312-328: LGTM! Error handling is well implemented.
The function properly handles different package types and includes appropriate error handling for invalid packages.
internal/exec/vendor_component_utils.go (2)
247-265: LGTM: Clean package type implementation
The package type determination and component package creation is well-structured and follows good practices.
333-346: LGTM: Well-implemented TUI handling
The TUI implementation with fallback for non-TTY environments is clean and user-friendly. Good job on providing a warning message when falling back to basic output.
internal/exec/vendor_utils.go (1)
73-83: LGTM! Flag handling is well-implemented.
The validation logic for the --everything flag is thorough and prevents invalid combinations with other flags.
| func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) { | ||
| return func(srcInfo os.FileInfo, src, dest string) (bool, error) { | ||
| if strings.HasSuffix(src, ".git") { | ||
| return true, nil | ||
| } | ||
|
|
||
| trimmedSrc := u.TrimBasePathFromPath(tempDir+"/", src) | ||
|
|
||
| // Exclude the files that match the 'excluded_paths' patterns | ||
| // It supports POSIX-style Globs for file names/paths (double-star `**` is supported) | ||
| // https://en.wikipedia.org/wiki/Glob_(programming) | ||
| // https://github.com/bmatcuk/doublestar#patterns | ||
| for _, excludePath := range s.ExcludedPaths { | ||
| excludeMatch, err := u.PathMatch(excludePath, src) | ||
| if err != nil { | ||
| return true, err | ||
| } else if excludeMatch { | ||
| // If the file matches ANY of the 'excluded_paths' patterns, exclude the file | ||
| u.LogTrace(cliConfig, fmt.Sprintf("Excluding the file '%s' since it matches the '%s' pattern from 'excluded_paths'\n", | ||
| trimmedSrc, | ||
| excludePath, | ||
| )) | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| // Only include the files that match the 'included_paths' patterns (if any pattern is specified) | ||
| if len(s.IncludedPaths) > 0 { | ||
| anyMatches := false | ||
| for _, includePath := range s.IncludedPaths { | ||
| includeMatch, err := u.PathMatch(includePath, src) | ||
| if err != nil { | ||
| return true, err | ||
| } else if includeMatch { | ||
| // If the file matches ANY of the 'included_paths' patterns, include the file | ||
| u.LogTrace(cliConfig, fmt.Sprintf("Including '%s' since it matches the '%s' pattern from 'included_paths'\n", | ||
| trimmedSrc, | ||
| includePath, | ||
| )) | ||
| anyMatches = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if anyMatches { | ||
| return false, nil | ||
| } else { | ||
| u.LogTrace(cliConfig, fmt.Sprintf("Excluding '%s' since it does not match any pattern from 'included_paths'\n", trimmedSrc)) | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| // If 'included_paths' is not provided, include all files that were not excluded | ||
| u.LogTrace(cliConfig, fmt.Sprintf("Including '%s'\n", u.TrimBasePathFromPath(tempDir+"/", src))) | ||
| return false, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Add path validation for security.
The skip function should validate paths to prevent path traversal attacks:
func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) {
return func(srcInfo os.FileInfo, src, dest string) (bool, error) {
+ // Validate path is within project boundary
+ if !strings.HasPrefix(filepath.Clean(src), filepath.Clean(tempDir)) {
+ return true, fmt.Errorf("path %s is outside of project boundary", src)
+ }
+
if strings.HasSuffix(src, ".git") {
return true, nil
}📝 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.
| func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) { | |
| return func(srcInfo os.FileInfo, src, dest string) (bool, error) { | |
| if strings.HasSuffix(src, ".git") { | |
| return true, nil | |
| } | |
| trimmedSrc := u.TrimBasePathFromPath(tempDir+"/", src) | |
| // Exclude the files that match the 'excluded_paths' patterns | |
| // It supports POSIX-style Globs for file names/paths (double-star `**` is supported) | |
| // https://en.wikipedia.org/wiki/Glob_(programming) | |
| // https://github.com/bmatcuk/doublestar#patterns | |
| for _, excludePath := range s.ExcludedPaths { | |
| excludeMatch, err := u.PathMatch(excludePath, src) | |
| if err != nil { | |
| return true, err | |
| } else if excludeMatch { | |
| // If the file matches ANY of the 'excluded_paths' patterns, exclude the file | |
| u.LogTrace(cliConfig, fmt.Sprintf("Excluding the file '%s' since it matches the '%s' pattern from 'excluded_paths'\n", | |
| trimmedSrc, | |
| excludePath, | |
| )) | |
| return true, nil | |
| } | |
| } | |
| // Only include the files that match the 'included_paths' patterns (if any pattern is specified) | |
| if len(s.IncludedPaths) > 0 { | |
| anyMatches := false | |
| for _, includePath := range s.IncludedPaths { | |
| includeMatch, err := u.PathMatch(includePath, src) | |
| if err != nil { | |
| return true, err | |
| } else if includeMatch { | |
| // If the file matches ANY of the 'included_paths' patterns, include the file | |
| u.LogTrace(cliConfig, fmt.Sprintf("Including '%s' since it matches the '%s' pattern from 'included_paths'\n", | |
| trimmedSrc, | |
| includePath, | |
| )) | |
| anyMatches = true | |
| break | |
| } | |
| } | |
| if anyMatches { | |
| return false, nil | |
| } else { | |
| u.LogTrace(cliConfig, fmt.Sprintf("Excluding '%s' since it does not match any pattern from 'included_paths'\n", trimmedSrc)) | |
| return true, nil | |
| } | |
| } | |
| // If 'included_paths' is not provided, include all files that were not excluded | |
| u.LogTrace(cliConfig, fmt.Sprintf("Including '%s'\n", u.TrimBasePathFromPath(tempDir+"/", src))) | |
| return false, nil | |
| } | |
| } | |
| func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) { | |
| return func(srcInfo os.FileInfo, src, dest string) (bool, error) { | |
| // Validate path is within project boundary | |
| if !strings.HasPrefix(filepath.Clean(src), filepath.Clean(tempDir)) { | |
| return true, fmt.Errorf("path %s is outside of project boundary", src) | |
| } | |
| if strings.HasSuffix(src, ".git") { | |
| return true, nil | |
| } | |
| trimmedSrc := u.TrimBasePathFromPath(tempDir+"/", src) | |
| // Exclude the files that match the 'excluded_paths' patterns | |
| // It supports POSIX-style Globs for file names/paths (double-star `**` is supported) | |
| // https://en.wikipedia.org/wiki/Glob_(programming) | |
| // https://github.com/bmatcuk/doublestar#patterns | |
| for _, excludePath := range s.ExcludedPaths { | |
| excludeMatch, err := u.PathMatch(excludePath, src) | |
| if err != nil { | |
| return true, err | |
| } else if excludeMatch { | |
| // If the file matches ANY of the 'excluded_paths' patterns, exclude the file | |
| u.LogTrace(cliConfig, fmt.Sprintf("Excluding the file '%s' since it matches the '%s' pattern from 'excluded_paths'\n", | |
| trimmedSrc, | |
| excludePath, | |
| )) | |
| return true, nil | |
| } | |
| } | |
| // Only include the files that match the 'included_paths' patterns (if any pattern is specified) | |
| if len(s.IncludedPaths) > 0 { | |
| anyMatches := false | |
| for _, includePath := range s.IncludedPaths { | |
| includeMatch, err := u.PathMatch(includePath, src) | |
| if err != nil { | |
| return true, err | |
| } else if includeMatch { | |
| // If the file matches ANY of the 'included_paths' patterns, include the file | |
| u.LogTrace(cliConfig, fmt.Sprintf("Including '%s' since it matches the '%s' pattern from 'included_paths'\n", | |
| trimmedSrc, | |
| includePath, | |
| )) | |
| anyMatches = true | |
| break | |
| } | |
| } | |
| if anyMatches { | |
| return false, nil | |
| } else { | |
| u.LogTrace(cliConfig, fmt.Sprintf("Excluding '%s' since it does not match any pattern from 'included_paths'\n", trimmedSrc)) | |
| return true, nil | |
| } | |
| } | |
| // If 'included_paths' is not provided, include all files that were not excluded | |
| u.LogTrace(cliConfig, fmt.Sprintf("Including '%s'\n", u.TrimBasePathFromPath(tempDir+"/", src))) | |
| return false, nil | |
| } | |
| } |
There was a problem hiding this comment.
@aknysh I am not sure if we should Validate path is within project boundary . what do you think about this?
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!
There was a problem hiding this comment.
We can validate if the provided path (e.g. components/terraform/xxxx is within the boundary of the components path (e.g. components/terraform) defined in atmos.yaml.
But at the same time, this will prevent the user to vendor into an arbitrary directory (which some of the users already do).
Let's not do it now and review in the future
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
website/docs/cheatsheets/vendoring.mdx (1)
Line range hint
70-76: Consider enhancing the command examples with descriptions, warrior! 🛡️While the command examples are comprehensive, they would benefit from brief descriptions explaining the purpose of each variant, especially the new
--everythingflag. This would help users understand when to use each option.Consider adding descriptions like this:
- atmos vendor pull --everything - atmos vendor pull --component vpc-mixin-1 - atmos vendor pull -c vpc-mixin-2 - atmos vendor pull -c vpc-mixin-3 - atmos vendor pull -c vpc-mixin-4 - atmos vendor pull --tags test - atmos vendor pull --tags networking,storage + # Pull all vendor components interactively + atmos vendor pull --everything + + # Pull specific components + atmos vendor pull --component vpc-mixin-1 + atmos vendor pull -c vpc-mixin-2 # Short form + + # Pull components by tags + atmos vendor pull --tags test + atmos vendor pull --tags networking,storagewebsite/docs/cheatsheets/commands.mdx (1)
193-196: Enhance the command description for clarity.The current description doesn't fully capture the new interactive capabilities introduced by this PR.
Consider updating the description to:
- <p>Pull sources and mixins from remote repositories for Terraform and Helmfile components and other artifacts</p> + <p>Interactive package manager to pull sources and mixins from remote repositories for Terraform and Helmfile components and other artifacts. Supports both TTY and non-TTY environments.</p>website/docs/cli/commands/vendor/vendor-pull.mdx (3)
Line range hint
117-122: Examples should demonstrate non-TTY usage.Based on the PR comments about TTY-related issues, it would be helpful to include examples of running the command in non-TTY mode.
Add examples for non-TTY usage:
atmos vendor pull --everything atmos vendor pull --component vpc atmos vendor pull -c vpc-flow-logs-bucket atmos vendor pull -c echo-server --type helmfile atmos vendor pull --tags dev,test atmos vendor pull --tags networking --dry-run +# For CI/CD pipelines or logging +atmos vendor pull --everything > vendor-pull.log 2>&1
Line range hint
127-135: Add debug logging information.Given the PR comment about debug logging not being helpful, we should document the debug capabilities.
Consider adding:
If `vendor.yaml` is not found, Atmos will look for the `component.yaml` manifest in the component's folder. If `component.yaml` is not found, an error will be thrown. The flag `--component` is required in this case + For troubleshooting, you can enable debug logging: + - Use `--logs-level Debug` to see detailed operation logs + - In non-TTY environments, logs are automatically written in a more parseable format + - The debug output includes progress information for each vendored component🧰 Tools
🪛 LanguageTool
[grammar] ~130-~130: The word ‘vendor’ is a noun or an adjective. A verb or adverb is missing or misspelled here, or maybe a comma is missing.
Context: ...componentis not specified, Atmos will vendor all the artifacts defined in thevendo...(PRP_MD_NN)
Line range hint
143-148: Update Flags table with logging options.The flags table should include logging-related flags mentioned in the PR comments.
Add the following rows to the flags table:
| Flag | Description | Alias | Required | |:--------------|:-------------------------------------------------------------------------------------------------------------|:------|:---------| | `--component` | Atmos component to pull | `-c` | no | | `--tags` | Only vendor the components that have the specified tags.<br/>`tags` is a comma-separated values (CSV) string | | no | | `--type` | Component type: `terraform` or `helmfile` (`terraform` is default) | `-t` | no | | `--dry-run` | Dry run | | no | +| `--logs-level`| Set logging level (Debug, Info, Warn, Error) | | no |website/docs/core-concepts/vendor/vendor-manifest.mdx (1)
414-414: Consider adding non-TTY mode example output.While the terminal output example is helpful, considering the PR objectives and the comment from user osterman about TTY issues, it would be valuable to include an example of running the command in non-TTY mode.
Add a section showing the command with output redirection:
> atmos vendor pull --everything Processing vendor config file 'vendor.yaml' ... + # Example for non-TTY mode + > atmos vendor pull --everything > vendor-pull.log 2>&1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
website/docs/cheatsheets/commands.mdx(1 hunks)website/docs/cheatsheets/vendoring.mdx(1 hunks)website/docs/cli/commands/vendor/vendor-pull.mdx(3 hunks)website/docs/core-concepts/vendor/vendor-manifest.mdx(3 hunks)
🔇 Additional comments (2)
website/docs/core-concepts/vendor/vendor-manifest.mdx (2)
30-30: LGTM! Command update aligns with PR objectives.
The addition of the --everything flag in the documentation accurately reflects the changes introduced in this PR for the interactive package manager interface.
157-157: LGTM! Comprehensive command examples.
The examples effectively demonstrate all supported command variations including the new --everything flag, component-specific pulls, and tag-based filtering.
|
@haitham911 please resolve the conflicts |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
internal/exec/vendor_component_utils.go (3)
98-105: Add more descriptive TODO commentThe TODO comment should provide more context about what needs to be implemented and any specific requirements or considerations.
-// TODO: implement this +// TODO: Implement stack vendoring support +// Requirements: +// - Process all components within the stack +// - Handle dependencies between components +// - Support parallel processing where possible
333-346: Consider adding fallback progress indicatorWhile the code correctly handles non-TTY environments by disabling the TUI renderer, it could benefit from a simple text-based progress indicator for better user experience in headless environments.
if !CheckTTYSupport() { opts = []tea.ProgramOption{tea.WithoutRenderer(), tea.WithInput(nil)} u.LogWarning(cliConfig, "TTY is not supported. Running in non-interactive mode") + // Add text-based progress handler for non-TTY environments + model.EnableTextProgress() }
352-356: Update TTY support check functionThe function name and comment don't accurately reflect that it checks both stdin and stdout TTY support. Also, the variable names are redundant.
-// CheckTTYSupport checks stdin support TTY. +// CheckTTYSupport checks if both stdin and stdout support TTY. func CheckTTYSupport() bool { - stdinTTYStdin := isatty.IsTerminal(os.Stdin.Fd()) - stdoutTTYStdout := isatty.IsTerminal(os.Stdout.Fd()) + stdinTTY := isatty.IsTerminal(os.Stdin.Fd()) + stdoutTTY := isatty.IsTerminal(os.Stdout.Fd()) - return stdinTTYStdin && stdoutTTYStdout + return stdinTTY && stdoutTTY }internal/exec/vendor_utils.go (2)
Line range hint
264-326: Consider adding package validation before processing.While the package creation logic is sound, consider validating package fields before appending to the packages slice:
- Validate target paths for potential conflicts
- Check for duplicate package names within the same source
328-344: Enhance error handling for TUI initialization.The TUI initialization could be improved:
- The error message on line 339 could be more descriptive
- Consider adding debug logging for non-TTY mode
- return fmt.Errorf("error initializing model: %v", err) + return fmt.Errorf("failed to initialize TUI model: %v (check terminal capabilities)", err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_utils.go(8 hunks)
🔇 Additional comments (4)
internal/exec/vendor_component_utils.go (2)
111-112: Fix .git directory exclusion logic
The current implementation may incorrectly exclude files or directories ending with .git. This was previously flagged in past reviews but hasn't been addressed.
-if strings.HasSuffix(src, ".git") {
+if filepath.Base(src) == ".git" {
return true, nil
}247-265: Enhance package type determination
The package type determination logic is clear and follows a good pattern. The code effectively handles OCI, local, and remote package types.
internal/exec/vendor_utils.go (2)
73-83: LGTM! Flag handling is well-implemented.
The validation logic for the --everything flag is thorough and prevents invalid flag combinations. The error messages are clear and helpful.
460-516:
Add path validation for security.
The skip function should validate paths to prevent path traversal attacks:
func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) {
return func(srcInfo os.FileInfo, src, dest string) (bool, error) {
+ // Validate path is within project boundary
+ if !strings.HasPrefix(filepath.Clean(src), filepath.Clean(tempDir)) {
+ return true, fmt.Errorf("path %s is outside of project boundary", src)
+ }
+
if strings.HasSuffix(src, ".git") {
return true, nil
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(2 hunks)internal/exec/vendor_model.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🔇 Additional comments (4)
internal/exec/vendor_model.go (4)
246-247: LGTM: Proper timeout handling for downloads.
The 30-minute timeout with proper context handling is a good practice to prevent hanging downloads.
320-328: LGTM: Proper error handling for invalid packages.
The error handling for cases where no valid package is provided is implemented correctly with appropriate error logging.
240-244:
Fix error handling and improve temp directory creation.
The error return type is inconsistent with the expected installedPkgMsg, and the temporary directory name could be more descriptive and secure.
-tempDir, err := os.MkdirTemp("", strconv.FormatInt(time.Now().Unix(), 10))
+tempDir, err := os.MkdirTemp("", fmt.Sprintf("atmos-vendor-%d-*", time.Now().Unix()))
if err != nil {
u.LogTrace(cliConfig, fmt.Sprintf("Failed to create temp directory %s", err))
- return err
+ return installedPkgMsg{
+ err: fmt.Errorf("failed to create temp directory: %w", err),
+ name: p.name,
+ }
}Likely invalid or redundant comment.
277-281:
Enhance symlink security in local file copy.
The current implementation follows symlinks without validation, which could potentially lead to security vulnerabilities if symlinks point to sensitive locations outside the intended directory structure.
✅ Verification successful
Symlink handling in local file copy is secure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potentially dangerous symlinks in the codebase
find . -type l -exec ls -l {} \; | grep -v "^l.*-> \."
Length of output: 59
Script:
#!/bin/bash
# Find symlinks that point outside the repository root
repo_root=$(pwd)
find . -type l | while read -r symlink; do
target=$(readlink -f "$symlink")
if [[ $target != $repo_root* ]]; then
echo "$symlink -> $target"
fi
done
Length of output: 220
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
internal/exec/vendor_utils.go (4)
74-83: Consider making the default behavior more explicit.The default behavior setting for the
everythingflag could be more explicit. Consider extracting this logic into a named function for better clarity.-// If neither `everything`, `component`, `stack`, nor `tags` flags are set, default to `everything = true` -if !everything && !flags.Changed("everything") && component == "" && stack == "" && len(tags) == 0 { - everything = true -} +func shouldDefaultToEverything(flags *pflag.FlagSet, everything bool, component, stack string, tags []string) bool { + return !everything && !flags.Changed("everything") && component == "" && stack == "" && len(tags) == 0 +} + +everything = shouldDefaultToEverything(flags, everything, component, stack, tags)
100-102: Enhance error message with actionable guidance.The error message could be more helpful by suggesting what the user should do next.
-return fmt.Errorf("the '--everything' flag is set, but the vendor config file '%s' does not exist", cfg.AtmosVendorConfigFileName) +return fmt.Errorf("the '--everything' flag is set, but the vendor config file '%s' does not exist. Create this file first or specify a component using '--component'", cfg.AtmosVendorConfigFileName)
343-344: Enhance error message for model initialization failure.The error message could be more descriptive to help with troubleshooting.
-return fmt.Errorf("error initializing model: %v", err) +return fmt.Errorf("failed to initialize TUI model: %v (check terminal capabilities and permissions)", err)
337-341: Enhance non-TTY mode logging.The non-TTY mode could benefit from more detailed logging to help users understand what's happening.
if !CheckTTYSupport() { opts = []tea.ProgramOption{tea.WithoutRenderer(), tea.WithInput(nil)} - u.LogWarning(cliConfig, "TTY is not supported. Running in non-interactive mode") + u.LogWarning(cliConfig, "No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") + u.LogInfo(cliConfig, "Progress will be logged as regular output messages") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(8 hunks)
🔇 Additional comments (1)
internal/exec/vendor_utils.go (1)
465-521:
Add path validation for security.
The skip function should validate paths to prevent path traversal attacks.
func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) {
return func(srcInfo os.FileInfo, src, dest string) (bool, error) {
+ // Validate path is within project boundary
+ cleanSrc := filepath.Clean(src)
+ cleanTemp := filepath.Clean(tempDir)
+ if !strings.HasPrefix(cleanSrc, cleanTemp) {
+ return true, fmt.Errorf("path %s is outside of project boundary", src)
+ }
+
if strings.HasSuffix(src, ".git") {Likely invalid or redundant comment.
… no packages are provided
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
internal/exec/vendor_model_component.go (4)
22-33: Consider adding field validationThe
pkgComponentVendorstruct looks well-organized, but consider adding validation for critical fields likeuriandcomponentPath. This could prevent issues with invalid paths or URIs later in the installation process.+func (p *pkgComponentVendor) validate() error { + if p.uri == "" { + return fmt.Errorf("uri cannot be empty") + } + if p.componentPath == "" { + return fmt.Errorf("componentPath cannot be empty") + } + return nil +}
35-65: Consider dynamic progress bar widthThe progress bar width is currently fixed at 30. Consider making it dynamic based on the terminal width for better user experience.
+ width, _, err := term.GetSize(int(os.Stdout.Fd())) + if err != nil { + width = 30 // fallback width + } p := progress.New( progress.WithDefaultGradient(), - progress.WithWidth(30), + progress.WithWidth(width/3), // Use a third of terminal width progress.WithoutPercentage(), )
112-184: Enhance error context with wrappingConsider wrapping errors with additional context to make debugging easier.
if err = client.Get(); err != nil { u.LogTrace(cliConfig, fmt.Sprintf("Failed to download package %s error %s", p.name, err)) - return err + return fmt.Errorf("failed to download package %s: %w", p.name, err) }
185-248: Consider reducing code duplication with component installationThe mixin installation shares similar code patterns with component installation, particularly around temporary directory creation and cleanup. Consider extracting these common patterns into helper functions.
+func createTempDir(cliConfig schema.CliConfiguration) (string, func(), error) { + tempDir, err := os.MkdirTemp("", strconv.FormatInt(time.Now().Unix(), 10)) + if err != nil { + return "", nil, fmt.Errorf("failed to create temp directory: %w", err) + } + cleanup := func() { + removeTempDir(cliConfig, tempDir) + } + return tempDir, cleanup, nil +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_model_component.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_model.go
🔇 Additional comments (1)
internal/exec/vendor_model_component.go (1)
67-111: LGTM! Download and installation logic looks solid
The function handles both components and mixins appropriately, with good error handling and dry run support.
|
@haitham911 please see the comments and resolve the conflicts |
| return cp.Copy(tempDir, targetPath, copyOptions) | ||
| } | ||
|
|
||
| func generateSkipFunction(cliConfig schema.CliConfiguration, tempDir string, s *schema.AtmosVendorSource) func(os.FileInfo, string, string) (bool, error) { |
There was a problem hiding this comment.
What does this function do? Can we add a comment
what
interactive shell for atmos vendor pull --everything
atmos vendor pull --everything </dev/null |& cat > atmos_vendor_pull.logscreenshots
why
Build an interface for a package manager using bubbletea
references
atmos vendor pull --tags ... --dry-rundoesn't do a full run #792Summary by CodeRabbit
New Features
--everythingflag for thevendorPullCmdcommand, allowing users to vendor all components in one go.--everythingoption in vendor commands.Bug Fixes
Refactor
Documentation
--everythingflag for vendor commands.