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📝 WalkthroughWalkthroughThe pull request introduces updates focused on dependency management in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)website/docs/core-concepts/vendor/vendor-manifest.mdx (2)
The command examples are well-structured and align with the PR objectives. However, as noted in the previous review, please restore the working example that was removed. Line range hint The documentation effectively covers all aspects of the vendor manifest functionality, including:
This will greatly help users understand and utilize the vendor pull functionality. Also applies to: 157-158 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.
|
Running 
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
internal/exec/vendor_component_utils.go (2)
98-105: Add more descriptive error message for unsupported stack vendor command.The error message could be more helpful by suggesting alternatives or explaining why it's not supported yet.
- return fmt.Errorf("command 'atmos vendor pull --stack <stack>' is not supported yet") + return fmt.Errorf("command 'atmos vendor pull --stack <stack>' is not supported yet. Please use 'atmos vendor pull --component <component>' instead")
340-353: Consider adding fallback progress indicator for non-TTY environments.While the code correctly handles non-TTY environments by disabling the TUI renderer, it could benefit from a simple text-based progress indicator as a fallback.
if !CheckTTYSupport() { opts = []tea.ProgramOption{tea.WithoutRenderer(), tea.WithInput(nil)} u.LogWarning(cliConfig, "TTY is not supported. Running in non-interactive mode") + // Add simple text-based progress updates + model.OnProgress = func(progress float64, status string) { + fmt.Printf("Progress: %.0f%% - %s\n", progress*100, status) + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/vendor_component_utils.go(4 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/vendor_component_utils.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#768
File: internal/exec/vendor_component_utils.go:312-316
Timestamp: 2024-11-20T18:38:34.525Z
Learning: Ensure that mixin package type determination supports local files, handling them consistently with component package type determination.
🔇 Additional comments (3)
internal/exec/vendor_component_utils.go (3)
111-113: Improve .git directory exclusion logic.
Using filepath.Base(src) == ".git" is more precise than string suffix matching.
247-265: LGTM: Clean package type determination logic.
The package type determination is well-structured and handles all cases (OCI, local, remote) appropriately.
359-361: LGTM: Simplified TTY support check.
The function correctly focuses on stdout TTY support since the vendoring process doesn't require user input.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
website/docs/core-concepts/vendor/vendor-manifest.mdx (1)
414-414: Consider enhancing the example output with timing information.The command output effectively demonstrates the hierarchical processing of vendor configurations. To make it even more informative for users, consider adding timing information to show how long each pull operation takes.
Add timing information using the
timeprefix:> atmos vendor pull --everything -Processing vendor config file 'vendor.yaml' -Pulling sources for the component 'my-vpc6' from 'github.com/cloudposse/terraform-aws-components.git//modules/vpc?ref=1.315.0' into 'components/terraform/infra/my-vpc6' +[00:00] Processing vendor config file 'vendor.yaml' +[00:01] Pulling sources for the component 'my-vpc6' from 'github.com/cloudposse/terraform-aws-components.git//modules/vpc?ref=1.315.0' into 'components/terraform/infra/my-vpc6'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
website/docs/cheatsheets/commands.mdx(1 hunks)website/docs/core-concepts/vendor/vendor-manifest.mdx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/docs/cheatsheets/commands.mdx
🔇 Additional comments (1)
website/docs/core-concepts/vendor/vendor-manifest.mdx (1)
157-157: LGTM! Example has been properly restored.
The CLI command examples are clear and demonstrate all supported usage patterns. The example that was previously requested to be restored is now present.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
website/docs/cli/commands/vendor/vendor-pull.mdx (1)
Line range hint
1-1: Add a troubleshooting section.Consider adding a troubleshooting section to help users handle common issues, such as TTY errors and debug logging. This would be particularly helpful given the reported issues.
Add this section at the end of the file:
## Troubleshooting ### Common Issues 1. TTY Errors - Error: "could not open a new TTY: open /dev/tty: no such device or address" - Solution: The command automatically falls back to non-interactive mode. Redirect output to a log file if needed. 2. Debug Logging - For detailed logging: Use `--logs-level Debug` - Example: `atmos vendor pull --everything --logs-level Debug` 3. CI/CD Environments - Always use non-interactive mode by redirecting output - Example: `atmos vendor pull --everything > vendor-pull.log 2>&1`website/docs/core-concepts/vendor/vendor-manifest.mdx (1)
157-158: Consider clarifying the default behavior ofatmos vendor pullSince we're introducing the
--everythingflag, it would be helpful to clarify the difference betweenatmos vendor pullandatmos vendor pull --everything. Consider adding a note explaining when to use each variant.-# atmos vendor pull +# Default behavior - requires specifying type, stack, or component +# atmos vendor pull + +# Pull all definitions # atmos vendor pull --everything
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
website/docs/cli/commands/vendor/vendor-pull.mdx(2 hunks)website/docs/core-concepts/vendor/vendor-manifest.mdx(2 hunks)
🔇 Additional comments (2)
website/docs/cli/commands/vendor/vendor-pull.mdx (1)
32-36: 🛠️ Refactor suggestion
Documentation needs to cover both interactive and non-TTY modes.
The command usage section should explain both the interactive mode (using Bubble Tea framework) and the non-TTY fallback behavior. This is particularly important given the reported issues with headless terminals.
Add a section explaining both modes:
atmos vendor pull
atmos vendor pull --everything
atmos vendor pull --component <component> [options]
atmos vendor pull -c <component> [options]
atmos vendor pull --tags <tag1>,<tag2> [options]
+
+## Terminal Modes
+
+The command supports two operational modes:
+
+1. Interactive Mode (Default for TTY)
+ - Provides a user-friendly interface with progress tracking
+ - Automatically enabled when running in a terminal
+
+2. Non-Interactive Mode
+ - Automatically used in non-TTY environments (CI/CD, redirected output)
+ - Suitable for logging and automation
+ - Example: `atmos vendor pull --everything > pull.log`website/docs/core-concepts/vendor/vendor-manifest.mdx (1)
415-415: LGTM! Terminal output example is comprehensive
The terminal output example effectively demonstrates:
- Usage of the new
--everythingflag - Pulling from multiple sources (OCI and Git)
- Version handling in targets
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
Summary by CodeRabbit
New Features
--everythingflag for thevendorPullCmdcommand, allowing users to vendor all components in one go.atmos.yaml) to utilize the new--everythingoption in vendor commands.vendor-manifest.mdx.Bug Fixes
Refactor
Documentation
--everythingflag for vendor commands.vendor-manifest.mdxdocumentation with detailed examples.