Implement: atmos list vendor #994
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new CLI command ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/list/list_vendor.go (1)
142-144: Sorting strategy could be refined.
Usingstrings.Joinon row columns might not match expected sorting on individual fields.cmd/list_vendor.go (1)
55-59: Consider direct test coverage.
You may add an integration or CLI test for this command’s flow to ensure consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/list_vendor.go(1 hunks)pkg/list/list_vendor.go(1 hunks)pkg/list/list_vendor_test.go(1 hunks)pkg/schema/schema.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/list/list_vendor_test.go
111-111: Error return value of os.Chdir is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/list/list_vendor.go (2)
21-27: Struct layout looks congruent.
No apparent issues with theVendorInfofields or JSON annotations.
44-55: Validate source component references.
Consider verifyingsource.Componentis non-empty to avoid potential folder creation issues.cmd/list_vendor.go (1)
15-23: Command usage is concise.
Arguments and examples are clear, supporting multiple formats.pkg/list/list_vendor_test.go (2)
18-240: Test coverage is thorough.
Your table-driven tests ensure multiple vendor formats are well-exercised.🧰 Tools
🪛 golangci-lint (1.62.2)
111-111: Error return value of
os.Chdiris not checked(errcheck)
111-111: Defer’s error return is unchecked.
It might be prudent to catchos.Chdir(currentDir)errors in a deferred function.Proposed fix:
- defer os.Chdir(currentDir) + defer func() { + if err := os.Chdir(currentDir); err != nil { + t.Logf("Error returning to original directory: %v", err) + } +}()🧰 Tools
🪛 golangci-lint (1.62.2)
111-111: Error return value of
os.Chdiris not checked(errcheck)
pkg/schema/schema.go (1)
637-638: LGTM! Clean addition of vendor list configuration.The addition of the
Listfield to theVendorstruct is well-structured and consistent with the existing codebase pattern, reusing theListConfigtype that's already used by theWorkflowsstruct. The field is properly tagged for YAML, JSON, and mapstructure serialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/list/list_vendor_test.go (2)
18-28: Consider extracting test setup into a helper function.The test setup code is well-structured, but extracting it into a helper function would improve reusability for future test cases.
+func setupTestEnvironment(t *testing.T) (string, func()) { + tmpDir, err := os.MkdirTemp("", "vendor_test") + require.NoError(t, err) + + // Create vendor directory structure + vendorDir := filepath.Join(tmpDir, "vendor.d") + err = os.MkdirAll(vendorDir, 0o755) + require.NoError(t, err) + + return tmpDir, func() { + os.RemoveAll(tmpDir) + } +} + func TestListVendors(t *testing.T) { - // Create a temporary directory for test files - tmpDir, err := os.MkdirTemp("", "vendor_test") - require.NoError(t, err) - defer os.RemoveAll(tmpDir) - - // Create vendor directory structure - vendorDir := filepath.Join(tmpDir, "vendor.d") - err = os.MkdirAll(vendorDir, 0o755) - require.NoError(t, err) + tmpDir, cleanup := setupTestEnvironment(t) + defer cleanup()
136-235: Consider adding edge cases to the test suite.The test cases are well-structured, but consider adding these scenarios:
- Empty vendor configuration
- Invalid format string
- Missing required columns
- Special characters in component names
I can help generate the additional test cases if you'd like.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_vendor_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/list/list_vendor_test.go
134-134: Error return value of os.Chdir is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [mock-macos] examples/demo-component-versions
- GitHub Check: [mock-macos] examples/demo-atlantis
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: [mock-linux] examples/demo-context
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [mock-linux] examples/demo-component-versions
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Docker Lint
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/list/list_vendor_test.go (2)
1-16: LGTM! Well-organized imports.The imports are properly organized and include all necessary packages for testing, including JSON handling, file operations, and testing utilities.
237-263: LGTM! Robust test execution.The test execution follows Go's table-driven test pattern with proper error handling and validation.
|
Note, |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/list/list_vendor.go (3)
21-28: Add field documentation for better code maintainability.While the struct itself is documented, adding documentation for each field would improve code maintainability.
// VendorInfo represents a vendor configuration entry type VendorInfo struct { + // Component is the name of the vendor component Component string `json:"component"` + // Type indicates the type of vendor configuration Type string `json:"type"` + // Manifest is the path to the vendor manifest file Manifest string `json:"manifest"` + // Folder is the path to the component folder Folder string `json:"folder"` + // Version specifies the version of the vendor component Version string `json:"version"` }
144-147: Improve sort function reliability.The current sort implementation could produce unexpected results when the delimiter appears in the data.
- sort.Slice(rows, func(i, j int) bool { - return strings.Join(rows[i], delimiter) < strings.Join(rows[j], delimiter) - }) + sort.Slice(rows, func(i, j int) bool { + // Compare by component first, then by type + if rows[i][0] != rows[j][0] { + return rows[i][0] < rows[j][0] + } + return rows[i][1] < rows[j][1] + })
171-188: Extract table styling logic for better maintainability.The table styling code should be extracted into a separate function for better organization.
+func createStyledTable(header []string, rows [][]string) string { + t := table.New(). + Border(lipgloss.ThickBorder()). + BorderStyle(lipgloss.NewStyle().Foreground(lipgloss.Color(theme.ColorBorder))). + StyleFunc(func(row, col int) lipgloss.Style { + style := lipgloss.NewStyle().PaddingLeft(1).PaddingRight(1) + if row == 0 { + return style.Inherit(theme.Styles.CommandName).Align(lipgloss.Center) + } + return style.Inherit(theme.Styles.Description) + }). + Headers(header...). + Rows(rows...) + return t.String() + utils.GetLineEnding() +} + // In FilterAndListVendors: if format == "" && exec.CheckTTYSupport() { - // Create a styled table for TTY - t := table.New(). - Border(lipgloss.ThickBorder()). - // ... rest of the table styling code ... - return t.String() + utils.GetLineEnding(), nil + return createStyledTable(header, rows), nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/list/list_vendor.go(1 hunks)pkg/list/list_vendor_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_vendor_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/list/list_vendor.go (1)
1-19: LGTM! Well-organized imports.The imports are properly organized and all are being used in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/list/list_workflows.go (2)
26-26: Consider enhancing format-specific delimiter handling.The addition of
FormatTSVis a good step, but we should ensure consistent delimiter handling between CSV and TSV formats.Consider defining format-specific delimiters as constants and using them in the format handling logic:
const ( FormatTable = "table" FormatJSON = "json" FormatCSV = "csv" FormatTSV = "tsv" + DelimiterCSV = "," + DelimiterTSV = "\t" )Also applies to: 34-34
192-199: Enhance format-specific output handling.The current implementation uses the same logic for both CSV and TSV formats. Consider separating these cases to handle format-specific requirements.
Consider updating the format handling:
switch format { case "json": // ... existing JSON handling ... case "csv": - // Use the provided delimiter for CSV output + // Use comma delimiter for CSV unless custom delimiter provided + csvDelimiter := DelimiterCSV + if delimiter != "" { + csvDelimiter = delimiter + } var output strings.Builder - output.WriteString(strings.Join(header, delimiter) + utils.GetLineEnding()) + output.WriteString(strings.Join(header, csvDelimiter) + utils.GetLineEnding()) for _, row := range rows { - output.WriteString(strings.Join(row, delimiter) + utils.GetLineEnding()) + output.WriteString(strings.Join(row, csvDelimiter) + utils.GetLineEnding()) } return output.String(), nil + case "tsv": + // Use tab delimiter for TSV unless custom delimiter provided + tsvDelimiter := DelimiterTSV + if delimiter != "" { + tsvDelimiter = delimiter + } + var output strings.Builder + output.WriteString(strings.Join(header, tsvDelimiter) + utils.GetLineEnding()) + for _, row := range rows { + output.WriteString(strings.Join(row, tsvDelimiter) + utils.GetLineEnding()) + } + return output.String(), nil
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/list_vendor.go(1 hunks)pkg/list/list_vendor.go(1 hunks)pkg/list/list_workflows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/list/list_vendor.go
- cmd/list_vendor.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
| } | ||
|
|
||
| // FilterAndListVendors lists vendor configurations based on the provided configuration | ||
| func FilterAndListVendors(listConfig schema.ListConfig, format string, delimiter string) (string, error) { |
There was a problem hiding this comment.
Please refactor into smaller functions. Under 50 lines long, with 10-30 ideal.
what
Listing all vendor config in tabular way
why
Evidence:
references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
atmos list vendorcommand, including usage examples and output options.Tests
Refactor
ProcessVendorImportsfunction for better accessibility across packages.