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 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 |
| rows = lo.UniqBy(rows, func(row []string) string { | ||
| return strings.Join(row, "\t") | ||
| }) | ||
| sort.Slice(rows, func(i, j int) bool { | ||
| return strings.Join(rows[i], "\t") < strings.Join(rows[j], "\t") | ||
| }) |
There was a problem hiding this comment.
Let's support an optional --delimiter flag, that defaults to \t
| Headers(header...). | ||
| Rows(rows...) | ||
|
|
||
| return t.String() + "\n", nil |
There was a problem hiding this comment.
I think we should use probably the OS new line characters
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.
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.