Implement: atmos list workflows #941
Conversation
|
|
Also add test cases |
osterman
added
minor
Test cases already added here cdd7a1e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Cerebrovinny
force-pushed
the
DEV-2801
branch
from
6ea2aa1 to
fdc63cb
Compare
📝 WalkthroughWalkthroughThe pull request introduces a new command Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ 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: 2
🧹 Nitpick comments (5)
pkg/list/list_workflows_test.go (1)
30-47: Reduce code duplication in tests.The test configuration and assertions are duplicated between the two test functions. Consider:
- Moving the common
ListConfigto a test helper- Creating a helper function for common assertions
+func createTestConfig() schema.ListConfig { + return schema.ListConfig{ + Columns: []schema.ListColumnConfig{ + {Name: "File", Value: "{{ .workflow_file }}"}, + {Name: "Workflow", Value: "{{ .workflow_name }}"}, + {Name: "Description", Value: "{{ .workflow_description }}"}, + }, + } +} + +func assertWorkflowOutput(t *testing.T, output string) { + expected := []string{ + "File", "Workflow", "Description", + "example", "test-1", "Test workflow", + } + for _, row := range expected { + assert.Contains(t, output, row) + } +}cmd/list_workflows.go (1)
20-22: Enhance command documentation with more examples.Consider adding examples for:
- Non-TTY usage (e.g., piping to grep)
- Different output formats
- File filtering patterns
- Example: "atmos list workflows\n" + - "atmos list workflows -f <file>", + Example: "# List all workflows\n" + + "atmos list workflows\n\n" + + "# Filter workflows by file\n" + + "atmos list workflows -f <file>\n\n" + + "# Use with pipes\n" + + "atmos list workflows | grep 'pattern'",pkg/list/list_workflows.go (2)
61-67: Optimize string operations in sorting.The current implementation concatenates strings repeatedly during sorting, which could be inefficient for large datasets.
-sort.Slice(rows, func(i, j int) bool { - return strings.Join(rows[i], "\t") < strings.Join(rows[j], "\t") -}) +sort.Slice(rows, func(i, j int) bool { + // Compare elements sequentially to avoid unnecessary string concatenation + for col := 0; col < len(rows[i]); col++ { + if rows[i][col] != rows[j][col] { + return rows[i][col] < rows[j][col] + } + } + return false +})
17-18: Add comprehensive documentation for exported functions.The exported functions need better documentation following Go standards.
-// getWorkflowsFromManifest extracts workflows from a workflow manifest +// getWorkflowsFromManifest extracts workflows from a workflow manifest and returns them as rows +// where each row contains [fileName, workflowName, description]. func getWorkflowsFromManifest(manifest schema.WorkflowManifest) ([][]string, error) { -// FilterAndListWorkflows filters and lists workflows based on the given file +// FilterAndListWorkflows filters and lists workflows based on the given file flag. +// It supports both TTY and non-TTY output formats, automatically degrading to a +// simple tabular format when TTY is not available. The output format can be +// customized using the listConfig parameter. func FilterAndListWorkflows(fileFlag string, listConfig schema.ListConfig) (string, error) {Also applies to: 30-31
pkg/schema/schema.go (1)
673-681: LGTM! Well-structured types for list configuration.The new types
ListConfigandListColumnConfigprovide a flexible configuration structure for the workflow listing feature. TheFormatfield will help support different output formats (TTY vs non-TTY), and the column configuration allows for customizable output.Consider adding Go doc comments to describe:
- Valid values for the
Formatfield- Expected format for the
Valuefield (e.g., if it supports templating)+// ListConfig defines the configuration for listing workflows +// Format specifies the output format (e.g., "table", "json") type ListConfig struct { Format string `yaml:"format" json:"format" mapstructure:"format"` Columns []ListColumnConfig `yaml:"columns" json:"columns" mapstructure:"columns"` } +// ListColumnConfig defines a column in the workflow list output +// Value can be a static string or a template expression type ListColumnConfig struct { Name string `yaml:"name" json:"name" mapstructure:"name"` Value string `yaml:"value" json:"value" mapstructure:"value"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/list_workflows.go(1 hunks)internal/tui/templates/term/term_writer.go(2 hunks)pkg/list/list_workflows.go(1 hunks)pkg/list/list_workflows_test.go(1 hunks)pkg/schema/schema.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/tui/templates/term/term_writer.go (1)
34-36: LGTM! Good standardization of TTY support check.The change to use
exec.CheckTTYSupport()aligns with our standardization efforts.pkg/list/list_workflows.go (1)
38-53: Replace example data with actual workflow manifest loading.The TODO comment and example data should be replaced with actual implementation.
Let's check if the workflow manifest loading is implemented elsewhere:
Would you like me to help implement the workflow manifest loading functionality or create a GitHub issue to track this?
✅ Verification successful
The workflow manifest loading implementation is indeed missing.
The schema for WorkflowManifest is properly defined in the codebase, but there's no actual implementation for loading workflow manifests from files. The example data should be replaced with proper manifest loading logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for workflow manifest loading implementations rg -l "WorkflowManifest.*Load|LoadWorkflow" --type goLength of output: 55
Script:
#!/bin/bash # Search for manifest-related code and configuration loading echo "=== Searching for manifest-related code ===" rg -l "manifest" --type go echo -e "\n=== Searching for YAML/JSON workflow config parsing ===" rg -l "yaml\.Unmarshal|json\.Unmarshal.*[Ww]orkflow" --type go echo -e "\n=== Checking schema package for manifest definitions ===" fd "schema.*\.go$" --exec cat {}Length of output: 41608
pkg/schema/schema.go (1)
137-138: LGTM! Field addition follows consistent patterns.The
Listfield is well-structured with appropriate tags, maintaining consistency with other fields in theWorkflowsstruct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/list/list_workflows_test.go (1)
31-75: 🛠️ Refactor suggestionStrengthen test coverage with additional test cases.
The test structure is robust, but consider adding these scenarios:
- Malformed YAML file to verify error handling
- Custom column configuration to verify template rendering
- Non-TTY output format (as requested in PR comments)
Here's a suggested addition to the test cases:
tests := []struct { // ... existing fields ... }{ // ... existing test cases ... + { + name: "malformed yaml", + fileFlag: "testdata/malformed.yaml", + config: schema.ListConfig{}, + wantErr: true, + }, + { + name: "custom columns", + fileFlag: "", + config: schema.ListConfig{ + Columns: []schema.ListColumnConfig{ + {Name: "Custom", Value: "{{ .workflow_name | upper }}"}, + }, + }, + contains: []string{"TEST-1"}, + }, + { + name: "non-tty output", + fileFlag: "", + config: schema.ListConfig{ + Format: "plain", + }, + contains: []string{"test-1\tTest workflow"}, + }, }
🧹 Nitpick comments (1)
pkg/list/list_workflows_test.go (1)
11-11: Consider upgrading to yaml.v3 for enhanced security and performance.The code currently uses
gopkg.in/yaml.v2. Version 3 of the yaml package offers better security features and improved performance.- "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/list/list_workflows.go(1 hunks)pkg/list/list_workflows_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_workflows.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_workflows_test.go (1)
105-144: 🛠️ Refactor suggestionRefactor test for better structure and stronger assertions.
This test has several opportunities for improvement:
- Extract common setup code to avoid duplication
- Use structured output verification instead of loose string checks
- Verify the exact format and order of the output
Here's a suggested refactor:
+func setupTestWorkflow(t *testing.T) (string, string) { + tmpDir, err := os.MkdirTemp("", "workflow_test") + require.NoError(t, err) + + testWorkflowFile := filepath.Join(tmpDir, "test.yaml") + // ... existing workflow setup code ... + + return tmpDir, testWorkflowFile +} + func TestListWorkflowsWithFile(t *testing.T) { - tmpDir, err := os.MkdirTemp("", "workflow_test") - require.NoError(t, err) + tmpDir, testWorkflowFile := setupTestWorkflow(t) defer os.RemoveAll(tmpDir) - - // Create a test workflow file - testWorkflowFile := filepath.Join(tmpDir, "test.yaml") // ... rest of the setup ... output, err := FilterAndListWorkflows(testWorkflowFile, listConfig) assert.NoError(t, err) - assert.Contains(t, output, "File") - assert.Contains(t, output, "Workflow") - // ... other Contains checks ... + + expected := []struct{ + header string + value string + }{ + {"File", "test.yaml"}, + {"Workflow", "test-1"}, + {"Description", "Test workflow"}, + } + + for _, e := range expected { + headerIdx := strings.Index(output, e.header) + valueIdx := strings.Index(output, e.value) + assert.Greater(t, valueIdx, headerIdx, + "Value %s should appear after header %s", e.value, e.header) + } }Likely invalid or redundant comment.
Note, if either flag is passed, TTY output is disabled. See |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/schema/schema.go (1)
696-704: Consider adding documentation for the new types.The
ListConfigandListColumnConfigtypes are well-structured and follow the codebase patterns. Consider adding documentation comments to describe:
- The purpose of each type
- Expected values for the
Formatfield- Examples of valid column configurations
Example documentation:
+// ListConfig defines the configuration for list command output formatting type ListConfig struct { Format string `yaml:"format" json:"format" mapstructure:"format"` Columns []ListColumnConfig `yaml:"columns" json:"columns" mapstructure:"columns"` } +// ListColumnConfig defines a column in the list output type ListColumnConfig struct { Name string `yaml:"name" json:"name" mapstructure:"name"` Value string `yaml:"value" json:"value" mapstructure:"value"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/schema/schema.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/schema/schema.go (1)
160-161: LGTM! Well-structured field addition.The
Listfield is properly integrated into theWorkflowsstruct with consistent tag formatting.
Cerebrovinny
force-pushed
the
DEV-2801
branch
from
7bdbbf6 to
e72cdea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/list/list_workflows.go (2)
88-93: Consider optimizing the sort key generation.The current implementation regenerates the sort key for each comparison.
-sort.Slice(rows, func(i, j int) bool { - return strings.Join(rows[i], delimiter) < strings.Join(rows[j], delimiter) -}) +// Pre-compute sort keys +type rowWithKey struct { + row []string + key string +} +rowsWithKeys := make([]rowWithKey, len(rows)) +for i, row := range rows { + rowsWithKeys[i] = rowWithKey{row: row, key: strings.Join(row, delimiter)} +} +sort.Slice(rowsWithKeys, func(i, j int) bool { + return rowsWithKeys[i].key < rowsWithKeys[j].key +}) +// Extract sorted rows +for i, rwk := range rowsWithKeys { + rows[i] = rwk.row +}
133-152: Consider extracting table styling into a separate function.The table styling logic could be moved to a dedicated function for better maintainability.
+func createStyledTable(header []string, rows [][]string) string { + return 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) + } + if row%2 == 0 { + return style.Inherit(theme.Styles.GrayText) + } + return style.Inherit(theme.Styles.Description) + }). + Headers(header...). + Rows(rows...). + String() + "\n" +}Then use it in the main function:
- t := table.New(). - Border(lipgloss.ThickBorder())... - return t.String() + "\n", nil + return createStyledTable(header, rows), nilcmd/list_workflows.go (1)
64-64: Enhance format flag documentation.The format flag description should list supported formats.
- listWorkflowsCmd.PersistentFlags().String("format", "", "Output format (table, json, csv)") + listWorkflowsCmd.PersistentFlags().String("format", "", "Output format. Supported values: 'table' (default), 'json', 'csv'")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/list_workflows.go(1 hunks)cmd/root.go(1 hunks)internal/tui/templates/term/term_writer.go(2 hunks)pkg/list/list_workflows.go(1 hunks)pkg/list/list_workflows_test.go(1 hunks)pkg/schema/schema.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_workflows_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/tui/templates/term/term_writer.go (1)
34-34: LGTM! Standardized TTY support check.The change to use
exec.CheckTTYSupport()aligns with the codebase's standardization efforts.cmd/root.go (1)
65-70: LGTM! Enhanced error handling for help commands.The improved error handling logic correctly suppresses config-not-found warnings during help commands while maintaining visibility for other scenarios.
pkg/list/list_workflows.go (1)
21-31: LGTM! Clean implementation of workflow extraction.The function effectively extracts workflow information into a consistent format.
pkg/schema/schema.go (1)
160-161: LGTM! Clean addition of list configuration to Workflows struct.The List field addition follows consistent struct tag patterns and integrates well with the existing schema.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
37-41: Consider providing default values for better user experience.The new workflow list configuration aligns well with the PR objectives, particularly supporting the
--formatflag requirement. However, consider providing sensible defaults:
- For
format: Consider a default that matches common CLI tools (e.g., "table" or "json")- For
columns: Consider defining default columns that would be most useful for typical workflow listingExample structure:
"list": { - "format": "", - "columns": null + "format": "table", + "columns": ["name", "description", "path"] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
37-41: Verify integration with terminal settings.The new workflow list configuration should respect the terminal settings defined in the
settings.terminalsection (e.g., colors, max_width, unicode support).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/list/list_workflows.go (3)
30-40: Consider adding input validationWhile the implementation is clean, consider adding defensive checks:
- Validate that manifest.Workflows is not nil
- Handle potential nil workflow descriptions
func getWorkflowsFromManifest(manifest schema.WorkflowManifest) ([][]string, error) { + if manifest.Workflows == nil { + return nil, fmt.Errorf("invalid manifest: workflows map is nil") + } var rows [][]string for workflowName, workflow := range manifest.Workflows { + description := workflow.Description + if description == "" { + description = "No description provided" + } rows = append(rows, []string{ manifest.Name, workflowName, - workflow.Description, + description, }) } return rows, nil }
73-87: Extract example data to a separate functionConsider extracting the example workflow manifest to a dedicated function for better maintainability and testability.
+func getExampleWorkflowManifest() schema.WorkflowManifest { + return schema.WorkflowManifest{ + Name: "example", + Workflows: schema.WorkflowConfig{ + "test-1": schema.WorkflowDefinition{ + Description: "Test workflow", + Steps: []schema.WorkflowStep{ + {Command: "echo Command 1", Name: "step1", Type: "shell"}, + {Command: "echo Command 2", Name: "step2", Type: "shell"}, + {Command: "echo Command 3", Name: "step3", Type: "shell"}, + {Command: "echo Command 4", Type: "shell"}, + }, + }, + }, + } +}
144-156: Extract style configuration to constantsConsider extracting the style configuration to named constants for better maintainability and reusability across the codebase.
+const ( + tablePaddingLeft = 1 + tablePaddingRight = 1 +) + +var tableHeaderStyle = lipgloss.NewStyle(). + PaddingLeft(tablePaddingLeft). + PaddingRight(tablePaddingRight) + 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) + style := tableHeaderStyle if row == 0 { return style.Inherit(theme.Styles.CommandName).Align(lipgloss.Center) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_workflows.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/list/list_workflows.go (2)
21-27: Well-implemented OS-specific line endings!The
LineEndingfunction correctly handles platform-specific line endings, which is crucial for cross-platform compatibility.
108-139: Excellent implementation of multiple output formats!The code effectively handles both JSON and CSV formats with proper delimiter support, making it versatile for different use cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
pkg/list/list_workflows.go (3)
27-39: Consider using a constant for valid formats.The validation logic is solid, but maintaining the valid formats list as a constant would improve maintainability.
+const ( + validFormats = []string{FormatTable, FormatJSON, FormatCSV} +) func ValidateFormat(format string) error { if format == "" { return nil } - validFormats := []string{FormatTable, FormatJSON, FormatCSV} for _, f := range validFormats { if format == f { return nil } } return fmt.Errorf("invalid format '%s'. Supported formats are: %s", format, strings.Join(validFormats, ", ")) }
104-118: Consider moving example workflow to test helpers.The example workflow could be moved to a test helper function to improve code organization and reusability.
143-147: Consider using constants for JSON field names.Using constants for JSON field names would improve maintainability and prevent typos.
+const ( + JSONFieldFile = "file" + JSONFieldName = "name" + JSONFieldDescription = "description" +) type workflow struct { - File string `json:"file"` - Name string `json:"name"` - Description string `json:"description"` + File string `json:"` + JSONFieldFile + `"` + Name string `json:"` + JSONFieldName + `"` + Description string `json:"` + JSONFieldDescription + `"` }pkg/list/list_workflows_test.go (2)
63-238: Consider adding more edge cases to the tests.While the test coverage is good, consider adding these scenarios:
- Malformed YAML file
- Very large workflow files
- Special characters in workflow names
240-332: Enhance test assertions for better validation.Consider adding more specific assertions:
- Verify exact content structure
- Validate field ordering
- Check for proper escaping in CSV format
pkg/schema/schema.go (1)
696-706: Consider adding more field validations.The schema could benefit from additional validations:
- Required fields validation
- Value template syntax validation
- Column name uniqueness check
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/list/list_workflows.go(1 hunks)pkg/list/list_workflows_test.go(1 hunks)pkg/schema/schema.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/list/list_workflows.go (4)
1-25: Clean package structure and format constants!The package organization and format constants are well-defined and follow Go conventions.
41-47: Well-implemented OS-specific line ending handling!The function correctly handles line endings for different operating systems.
171-202: Excellent table formatting with proper TTY handling!The table formatting implementation is robust with proper styling and graceful fallback for non-TTY environments.
82-86:⚠️ Potential issueAdd path validation for security.
The file path should be validated to prevent directory traversal attacks.
if fileFlag != "" { + // Validate file path + cleanPath := filepath.Clean(fileFlag) + if !strings.HasSuffix(cleanPath, ".yaml") && !strings.HasSuffix(cleanPath, ".yml") { + return "", fmt.Errorf("invalid workflow file extension: %s", fileFlag) + } + if _, err := os.Stat(fileFlag); os.IsNotExist(err) { return "", fmt.Errorf("workflow file not found: %s", fileFlag) }Likely invalid or redundant comment.
pkg/list/list_workflows_test.go (1)
18-61: Comprehensive format validation tests!The test cases cover all scenarios including empty format, valid formats, and invalid format.
pkg/schema/schema.go (1)
160-161: Clean integration of ListConfig field!The List field is well-placed in the Workflows struct with proper tags.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/list/list_workflows.go (4)
22-40: Consider using a map for format validation.While the current implementation is correct, we can optimize the format validation by using a map for O(1) lookup instead of iterating through a slice.
const ( FormatTable = "table" FormatJSON = "json" FormatCSV = "csv" ) +var validFormats = map[string]bool{ + FormatTable: true, + FormatJSON: true, + FormatCSV: true, +} + // ValidateFormat checks if the given format is supported func ValidateFormat(format string) error { if format == "" { return nil } - validFormats := []string{FormatTable, FormatJSON, FormatCSV} - for _, f := range validFormats { - if format == f { - return nil - } + if validFormats[format] { + return nil } + formats := []string{FormatTable, FormatJSON, FormatCSV} - return fmt.Errorf("invalid format '%s'. Supported formats are: %s", format, strings.Join(validFormats, ", ")) + return fmt.Errorf("invalid format '%s'. Supported formats are: %s", format, strings.Join(formats, ", ")) }
42-48: Consider defining line ending constants.The implementation is solid, but we could make it more maintainable by defining constants for the line endings.
+const ( + windowsLineEnding = "\r\n" + unixLineEnding = "\n" +) + // LineEnding returns the appropriate line ending for the current OS func LineEnding() string { if runtime.GOOS == "windows" { - return "\r\n" + return windowsLineEnding } - return "\n" + return unixLineEnding }
184-200: Extract table styling to a separate function.The table styling logic could be moved to a separate function to improve maintainability.
+func createStyledTable(header []string, rows [][]string) *table.Table { + return 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) + } + if row%2 == 0 { + return style.Inherit(theme.Styles.GrayText) + } + return style.Inherit(theme.Styles.Description) + }). + Headers(header...). + Rows(rows...) +} + if format == "" && exec.CheckTTYSupport() { - // Create a styled table for TTY - 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) - } - if row%2 == 0 { - return style.Inherit(theme.Styles.GrayText) - } - return style.Inherit(theme.Styles.Description) - }). - Headers(header...). - Rows(rows...) + t := createStyledTable(header, rows)
114-127: Move example workflow to a constant or separate function.The example workflow could be moved to a constant or separate function to improve maintainability and make it easier to update.
+func getExampleWorkflow() schema.WorkflowManifest { + return schema.WorkflowManifest{ + Name: "example", + Workflows: schema.WorkflowConfig{ + "test-1": schema.WorkflowDefinition{ + Description: "Test workflow", + Steps: []schema.WorkflowStep{ + {Command: "echo Command 1", Name: "step1", Type: "shell"}, + {Command: "echo Command 2", Name: "step2", Type: "shell"}, + {Command: "echo Command 3", Name: "step3", Type: "shell"}, + {Command: "echo Command 4", Type: "shell"}, + }, + }, + }, + } +} + - manifest := schema.WorkflowManifest{ - Name: "example", - Workflows: schema.WorkflowConfig{ - "test-1": schema.WorkflowDefinition{ - Description: "Test workflow", - Steps: []schema.WorkflowStep{ - {Command: "echo Command 1", Name: "step1", Type: "shell"}, - {Command: "echo Command 2", Name: "step2", Type: "shell"}, - {Command: "echo Command 3", Name: "step3", Type: "shell"}, - {Command: "echo Command 4", Type: "shell"}, - }, - }, - }, - } + manifest := getExampleWorkflow()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_workflows.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-vendoring
- 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: Acceptance Tests (macos-latest, macos)
- 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: Summary
🔇 Additional comments (2)
pkg/list/list_workflows.go (2)
1-20: LGTM! Clean package structure and well-organized imports.The imports are properly grouped and all are being utilized in the implementation.
50-64: LGTM! Robust workflow extraction with proper nil checks.The implementation correctly handles nil workflows and follows the suggestions from previous reviews.
| if !strings.HasSuffix(cleanPath, ".yaml") && !strings.HasSuffix(cleanPath, ".yml") { | ||
| return "", fmt.Errorf("invalid workflow file extension: %s", fileFlag) | ||
| } |
There was a problem hiding this comment.
We are doing this everywhere, I feel like handling multiple extensions should be generalized.
There was a problem hiding this comment.
we also already have the strings .yaml and .yml as global constants, they need to be used in the code
| // Expected line ending based on OS | ||
| expectedLineEnding := "\n" | ||
| if runtime.GOOS == "windows" { | ||
| expectedLineEnding = "\r\n" | ||
| } |
There was a problem hiding this comment.
Should be something in one of our utility packages
| // Expected line ending based on OS | ||
| expectedLineEnding := "\n" | ||
| if runtime.GOOS == "windows" { | ||
| expectedLineEnding = "\r\n" | ||
| } |
what
Atmos list workflow with the standard UI
why
Users should be able to list their workflows using this command
Updated 01/16
references
Summary by CodeRabbit
Release Notes
New Features
Improvements
Testing
The release introduces workflow listing capabilities with enhanced display and configuration options for Atmos users.