feat: Make workflow --file flag optional with auto-discovery #1800
Conversation
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR restructures the workflow command from a monolithic implementation in Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (cmd/workflow)
participant Config as Config Engine
participant FileSystem as File System
participant UI as UI/TTY
participant WorkflowEngine as Workflow Engine
User->>CLI: atmos workflow myworkflow
CLI->>Config: Load Atmos config
Config-->>CLI: config ready
alt Workflow file provided via --file
CLI->>WorkflowEngine: Execute workflow
else Auto-discover workflow files
CLI->>FileSystem: Search for workflow files
FileSystem-->>CLI: matching files
alt Single match found
CLI->>WorkflowEngine: Execute workflow (auto-selected file)
else Multiple matches found
alt TTY/Interactive mode
CLI->>UI: Show workflow selector
UI->>User: Prompt user to select file
User-->>UI: User selection
UI-->>CLI: Selected file
CLI->>WorkflowEngine: Execute workflow
else CI/Non-interactive mode
CLI-->>User: Error: multiple files, specify --file
end
else No matches found
CLI-->>User: Error: workflow not found
end
end
WorkflowEngine-->>CLI: Execution result or error
CLI-->>User: Formatted output/error with hints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
osterman
changed the title
osterman
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1800 +/- ##
==========================================
- Coverage 71.45% 71.33% -0.12%
==========================================
Files 461 462 +1
Lines 43631 43780 +149
==========================================
+ Hits 31175 31229 +54
- Misses 9911 10001 +90
- Partials 2545 2550 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
website/blog/2025-11-18-workflow-file-auto-discovery.md (1)
72-110: Consider calling out non-interactive (CI) behavior when duplicates exist.You explain that duplicate workflow names trigger an interactive selector, but in non-TTY contexts (e.g., CI) selection will fail and require
--file. Adding a short note under “How It Works” or “Backward Compatibility” that non-interactive runs error on duplicates and must pass--filewould make the behavior clearer for CI users.cmd/workflow/utils.go (2)
54-56: Fix godot-style comment punctuation.The first line of the multi-line comment above
identityFlagCompletiondoes not end with a period, which will trip the godot linter.You can tweak it to something like:
-// identityFlagCompletion provides shell completion for identity flags by fetching -// available identities from the Atmos configuration. +// identityFlagCompletion provides shell completion for identity flags by fetching. +// Available identities from the Atmos configuration.or otherwise ensure each
//line ends with a period.
28-35: Optional: consider prefix-filtering completion results usingtoComplete.All three completion functions currently return the full set of candidates, ignoring the
toCompleteprefix. That’s safe, but lightly filtering on the client side can reduce noise for users with many stacks/identities/workflows and shouldn’t affect correctness.For example:
-import ( - "sort" +import ( + "sort" + "strings" @@ func stackFlagCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { @@ - return output, cobra.ShellCompDirectiveNoFileComp + if toComplete == "" { + return output, cobra.ShellCompDirectiveNoFileComp + } + + var filtered []string + for _, s := range output { + if strings.HasPrefix(s, toComplete) { + filtered = append(filtered, s) + } + } + return filtered, cobra.ShellCompDirectiveNoFileComp @@ func identityFlagCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { @@ - return identities, cobra.ShellCompDirectiveNoFileComp + if toComplete == "" { + return identities, cobra.ShellCompDirectiveNoFileComp + } + + var filtered []string + for _, id := range identities { + if strings.HasPrefix(id, toComplete) { + filtered = append(filtered, id) + } + } + return filtered, cobra.ShellCompDirectiveNoFileComp @@ func workflowNameCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { @@ - return completions, cobra.ShellCompDirectiveNoFileComp + if toComplete == "" { + return completions, cobra.ShellCompDirectiveNoFileComp + } + + var filtered []string + for _, c := range completions { + // Match on the value before the tab (workflow name). + if name, _, ok := strings.Cut(c, "\t"); ok { + if strings.HasPrefix(name, toComplete) { + filtered = append(filtered, c) + } + } + } + return filtered, cobra.ShellCompDirectiveNoFileCompFeel free to adjust matching semantics (case sensitivity, etc.) to align with other completions in the repo.
Also applies to: 56-72, 85-120
internal/exec/workflow_test.go (1)
384-417: Optional: factor out repeatedstacksPath/env wiring in these subtests.Several subtests repeat the same
stacksPath+t.Setenvboilerplate before callingcreateWorkflowCmdandExecuteWorkflowCmd. If this file grows further, consider a small helper (e.g.,setupWorkflowTestEnv(t *testing.T) *cobra.Command) to centralize that setup and reduce duplication, but it’s not urgent.cmd/workflow/utils_test.go (2)
45-54: Consider adding explicit assertions or documenting intent.Lines 50-53 discard both
errandresults. If the intent is just "doesn't panic," add a brief comment. If fixtures should return valid stacks, assert on the results.- // May error if the fixtures don't have valid stacks configured. - // The important thing is that the function runs without panicking. - _ = err - _ = results + // Validates function executes without panicking. + // Fixtures may not have valid stacks configured; error handling is acceptable. + assert.True(t, err == nil || err != nil, "function should complete without panic")
276-284: Conditional assertion may mask test failures.If
resultsis empty, the assertions inside theifblock are skipped, and the test passes. Consider asserting unconditionally:assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive) - if len(results) > 0 { - assert.Len(t, results, 2) - // Should have both entries with different file contexts. - assert.Contains(t, results, "deploy\tfile1.yaml") - assert.Contains(t, results, "deploy\tfile2.yaml") - } + require.Len(t, results, 2, "expected both workflow files to be discovered") + assert.Contains(t, results, "deploy\tfile1.yaml") + assert.Contains(t, results, "deploy\tfile2.yaml")cmd/workflow/workflow.go (2)
129-133: Unusedoptsparameter.
WorkflowOptionsis parsed from flags on lines 58-65 but passed as_(unused) toexecuteWorkflowWithOptions. Either integrate the options or simplify by removing the parsing step.-func executeWorkflowWithOptions(cmd *cobra.Command, args []string, _ *WorkflowOptions) error { +func executeWorkflowWithOptions(cmd *cobra.Command, args []string, opts *WorkflowOptions) error { + // TODO: Use opts directly instead of re-parsing flags in ExecuteWorkflowCmd return e.ExecuteWorkflowCmd(cmd, args) }
135-141: Redundant help handling.Cobra handles
--helpand-hautomatically. This function would also match a workflow literally namedhelp. Consider removing unless there's a specific use case.-// handleHelpRequest handles the help request for the workflow command. -func handleHelpRequest(cmd *cobra.Command, args []string) { - // Check if help is requested. - if len(args) > 0 && (args[0] == "help" || args[0] == "--help" || args[0] == "-h") { - _ = cmd.Help() - } -}internal/exec/workflow_utils.go (1)
459-462: TTY check is redundant but defensive.The caller in
workflow.goalready checks TTY/CI status before calling this function. The duplicate check here is harmless but adds unnecessary code. Consider documenting the precondition or removing the check.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/exec/workflow_utils.go(10 hunks)tests/test-cases/workflows.yaml(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/test-cases/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER modify tests/test-cases/ or tests/testdata/ unless explicitly instructed. Golden snapshots are sensitive to minor changes.
Files:
tests/test-cases/workflows.yaml
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use functional options pattern to avoid functions with many parameters. Define Option functions that modify a Config struct and pass variadic options to New functions.
Use context.Context for cancellation signals, deadlines/timeouts, and request-scoped values (trace IDs). Context should be first parameter in functions that accept it. DO NOT use context for configuration, dependencies, or avoiding proper function parameters.
All comments must end with periods. This is enforced by the godot linter.
NEVER delete existing comments without a very strong reason. Preserve helpful comments, update them to match code changes, refactor for clarity, and add context when modifying code. Only remove factually incorrect, duplicated, or outdated comments.
Organize imports in three groups separated by blank lines (Go stdlib, 3rd-party excluding cloudposse/atmos, Atmos packages), sorted alphabetically. Maintain aliases: cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions. Use nil if no atmosConfig parameter.
All errors MUST be wrapped using static errors defined in errors/errors.go. Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, and errors.Is() for error checking. NEVER use dynamic errors directly.
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation. Never create manual mocks.
Use viper.BindEnv with ATMOS_ prefix for environment variables.
Use colors from pkg/ui/theme/colors.go for theme consistency.
Ensure Linux/macOS/Windows compatibility. Use SDKs over binaries. Use filepath.Join(), not hardcoded path separators.
Small focused files (<600 lines). One cmd/impl per file. Co-locate tests. Never use //revive:disable:file-length-limit.
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider us...
Files:
internal/exec/workflow_utils.go
🧠 Learnings (23)
📓 Common learnings
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning specific error variables (like `ErrWorkflowNoSteps`, `ErrInvalidFromStep`, `ErrInvalidWorkflowStepType`, `ErrWorkflowStepFailed`) instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions while maintaining excellent user-facing error formatting.
Applied to files:
tests/test-cases/workflows.yamlinternal/exec/workflow_utils.go
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
Repo: cloudposse/atmos PR: 1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.710Z
Learning: In the atmos codebase, workflow error handling was refactored to use `PrintErrorMarkdown` followed by returning the error instead of `PrintErrorMarkdownAndExit`. This pattern allows proper error testing without the function terminating the process with `os.Exit`, enabling unit tests to assert on error conditions.
Applied to files:
tests/test-cases/workflows.yamlinternal/exec/workflow_utils.go
📚 Learning: 2025-10-11T19:12:38.832Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden:0-0
Timestamp: 2025-10-11T19:12:38.832Z
Learning: Usage Examples sections in error output are appropriate for command usage errors (incorrect syntax, missing arguments, invalid flags) but not for configuration validation errors (malformed workflow files, invalid settings in atmos.yaml). Configuration errors should focus on explaining what's wrong with the config, not command usage patterns.
Applied to files:
tests/test-cases/workflows.yaml
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-11-24T17:35:20.297Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:35:20.297Z
Learning: Applies to **/*.go : Organize imports in three groups separated by blank lines (Go stdlib, 3rd-party excluding cloudposse/atmos, Atmos packages), sorted alphabetically. Maintain aliases: cfg, log, u, errUtils.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-11-24T17:35:20.297Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:35:20.297Z
Learning: Separate I/O (streams) from UI (formatting) using pkg/io/ and pkg/ui/ packages. Use data.Write/data.WriteJSON/data.WriteYAML for pipeable output (stdout), ui.Write/ui.Success/ui.Error for UI messages (stderr), and ui.Markdown for rendered documentation.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-10-21T17:51:07.087Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:07.087Z
Learning: Use `bubbletea` for confirmation prompts instead of `fmt.Scanln` in the `atmos terraform clean` command.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-11-24T17:35:20.297Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:35:20.297Z
Learning: Applies to **/*.go : Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions. Use nil if no atmosConfig parameter.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-05-30T03:21:37.197Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1274
File: go.mod:63-63
Timestamp: 2025-05-30T03:21:37.197Z
Learning: The redis dependency (github.com/redis/go-redis/v9) in the atmos project is only used in tests, not in production code.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-02-06T13:38:07.216Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-12-05T22:33:40.955Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 820
File: cmd/list_components.go:53-54
Timestamp: 2024-12-05T22:33:40.955Z
Learning: In the Atmos CLI Go codebase, using `u.LogErrorAndExit` within completion functions is acceptable because it logs the error and exits the command execution.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2024-12-05T22:33:54.807Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 820
File: cmd/list_stacks.go:55-56
Timestamp: 2024-12-05T22:33:54.807Z
Learning: In the atmos project, the `u.LogErrorAndExit` function logs the error and exits the command execution appropriately within flag completion functions.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-01-30T19:30:59.120Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/workflow.go:74-74
Timestamp: 2025-01-30T19:30:59.120Z
Learning: Error handling for `cmd.Usage()` is not required in the Atmos CLI codebase, as confirmed by the maintainer.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
internal/exec/workflow_utils.go
📚 Learning: 2025-02-03T06:00:11.419Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 959
File: cmd/describe_config.go:20-20
Timestamp: 2025-02-03T06:00:11.419Z
Learning: Commands should use `PrintErrorMarkdownAndExit` with empty title and suggestion (`"", err, ""`) for general error handling. Specific titles like "Invalid Usage" or "File Not Found" should only be used for validation or specific error scenarios.
Applied to files:
internal/exec/workflow_utils.go
🧬 Code graph analysis (1)
internal/exec/workflow_utils.go (8)
errors/builder.go (1)
Build(21-23)errors/exit_code.go (2)
WithExitCode(40-48)GetExitCode(60-96)internal/exec/shell_utils.go (1)
ExecuteShellCommand(34-106)pkg/schema/workflow.go (1)
WorkflowManifest(24-28)internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStdin(133-135)pkg/telemetry/ci.go (1)
IsCI(81-86)pkg/ui/markdown/renderer.go (1)
Option(273-273)internal/tui/utils/utils.go (1)
NewAtmosHuhTheme(98-106)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (14)
tests/test-cases/workflows.yaml (4)
49-51: Hints formatting looks consistent across execution errors.The "## Hints" sections for workflow step failures provide useful context (failed command and resume guidance). The pattern is applied consistently for shell command errors and failures.
Also applies to: 73-75, 94-96, 252-254, 272-274
134-136: New step-type hints are helpful and specific.The hints clarifying unsupported step type and valid options ("atmos" or "shell") provide clear guidance for fixing configuration errors.
178-182: Verify header consistency: "# Error" vs "Workflow Error".The "no workflow found" error uses "# Error" and shows available workflows in hints, which aligns with the auto-discovery feature. However, this differs from other workflow configuration errors (line 113 uses "Workflow Error"). Ensure this inconsistency is intentional—whether generic "# Error" is used for file/lookup failures vs. "Workflow Error" for validation failures.
206-211: Verify error header consistency for file-related errors.Both "invalid workflow manifest" (line 206) and "workflow file not found" (line 228) changed to "# Error" headers and "## Hints" sections. Confirm this represents the intended categorization and output format from the refactored error handling code.
Also applies to: 228-232
internal/exec/workflow_utils.go (10)
3-32: Imports look good.The three-group organization (stdlib, 3rd-party, Atmos) is correct and alphabetically sorted. Standard aliases are properly maintained.
44-45: Well-defined static errors.Both new error definitions follow the project pattern and provide clear, actionable messages.
290-310: Excellent error handling with proper wrapping and exit code preservation.The error construction correctly:
- Wraps the underlying error with
ErrWorkflowStepFailedusingfmt.Errorf("%w: %w", ...)to preserve both sentinel and exit code- Extracts and preserves the exit code from the underlying error
- Formats commands in code fences for proper rendering
- Uses single quotes for shell safety
This aligns perfectly with the learnings about workflow error handling.
Based on learnings, this follows the established pattern of using
errUtils.Buildinstead ofPrintErrorMarkdownAndExitfor testability.
366-384: Good resilience improvements.Skipping unreadable or invalid workflow files with warnings is a solid UX improvement. Users can still see available workflows while being informed of issues.
412-417: Clean type definition.
WorkflowMatchis well-documented and serves its purpose for auto-discovery results.
419-449: Function implementation looks solid.The workflow discovery logic is straightforward and correctly uses
ExecuteDescribeWorkflows. Performance tracking is properly in place.
459-461: Good environment checks.The combination of TTY and CI detection ensures interactive selection only runs in appropriate environments.
484-498: Nice UX touches.Adding ESC to the quit keys and using the Atmos theme creates a consistent, user-friendly experience.
500-508: Proper error handling for user interactions.Converting
huh.ErrUserAbortedtoerrUtils.ErrUserAbortedmaintains consistency with the rest of the codebase.
246-246: Based on my verification, I can now provide the rewritten review comment:Add comment explaining why
ui.Infoferror is being discarded on line 246.The error return from
ui.Infofis intentionally being discarded. While this appears to be best-effort logging before executing a workflow step (which is a reasonable pattern used elsewhere in the codebase), the lack of an explanatory comment is inconsistent with similar best-effort patterns in the codebase (e.g.,pkg/auth/manager.go:_ = m.ensureIdentityHasManager(identityName)with comment "This is best-effort..."). Add a comment explaining why the error is safely ignored, such as:// Logging is best-effort; if it fails, continue with workflow execution.
| // promptForWorkflowFile shows an interactive selector for choosing a workflow file. | ||
| // Uses the Huh library with Atmos theme (same pattern as identity selector). | ||
| func promptForWorkflowFile(matches []WorkflowMatch) (string, error) { | ||
| if len(matches) == 0 { | ||
| return "", ErrNoWorkflowFilesToSelect | ||
| } | ||
|
|
||
| // Check if we're in a TTY environment. | ||
| if !term.IsTTYSupportForStdin() || telemetry.IsCI() { | ||
| return "", ErrNonTTYWorkflowSelection | ||
| } | ||
|
|
||
| // Sort matches alphabetically by file name for consistent ordering. | ||
| sortedMatches := make([]WorkflowMatch, len(matches)) | ||
| copy(sortedMatches, matches) | ||
| sort.Slice(sortedMatches, func(i, j int) bool { | ||
| return sortedMatches[i].File < sortedMatches[j].File | ||
| }) | ||
|
|
||
| // Build options for the selector. | ||
| // Each option shows the file name with description if available. | ||
| options := make([]huh.Option[string], len(sortedMatches)) | ||
| for i, match := range sortedMatches { | ||
| label := match.File | ||
| if match.Description != "" { | ||
| label = fmt.Sprintf("%s - %s", match.File, match.Description) | ||
| } | ||
| options[i] = huh.NewOption(label, match.File) | ||
| } | ||
|
|
||
| var selectedFile string | ||
|
|
||
| // Create custom keymap that adds ESC to quit keys. | ||
| keyMap := huh.NewDefaultKeyMap() | ||
| keyMap.Quit = key.NewBinding( | ||
| key.WithKeys("ctrl+c", "esc"), | ||
| key.WithHelp("ctrl+c/esc", "quit"), | ||
| ) | ||
|
|
||
| form := huh.NewForm( | ||
| huh.NewGroup( | ||
| huh.NewSelect[string](). | ||
| Title(fmt.Sprintf("Multiple workflows found with name '%s'. Please choose:", sortedMatches[0].Name)). | ||
| Description("Press ctrl+c or esc to exit"). | ||
| Options(options...). | ||
| Value(&selectedFile), | ||
| ), | ||
| ).WithKeyMap(keyMap).WithTheme(uiutils.NewAtmosHuhTheme()) | ||
|
|
||
| if err := form.Run(); err != nil { | ||
| if errors.Is(err, huh.ErrUserAborted) { | ||
| return "", errUtils.ErrUserAborted | ||
| } | ||
| return "", fmt.Errorf("workflow selection failed: %w", err) | ||
| } | ||
|
|
||
| return selectedFile, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add performance tracking.
This public function is missing the required defer perf.Track(nil, "exec.promptForWorkflowFile")() call.
As per coding guidelines, add performance tracking at the beginning of the function:
func promptForWorkflowFile(matches []WorkflowMatch) (string, error) {
+ defer perf.Track(nil, "exec.promptForWorkflowFile")()
+
if len(matches) == 0 {📝 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.
| // promptForWorkflowFile shows an interactive selector for choosing a workflow file. | |
| // Uses the Huh library with Atmos theme (same pattern as identity selector). | |
| func promptForWorkflowFile(matches []WorkflowMatch) (string, error) { | |
| if len(matches) == 0 { | |
| return "", ErrNoWorkflowFilesToSelect | |
| } | |
| // Check if we're in a TTY environment. | |
| if !term.IsTTYSupportForStdin() || telemetry.IsCI() { | |
| return "", ErrNonTTYWorkflowSelection | |
| } | |
| // Sort matches alphabetically by file name for consistent ordering. | |
| sortedMatches := make([]WorkflowMatch, len(matches)) | |
| copy(sortedMatches, matches) | |
| sort.Slice(sortedMatches, func(i, j int) bool { | |
| return sortedMatches[i].File < sortedMatches[j].File | |
| }) | |
| // Build options for the selector. | |
| // Each option shows the file name with description if available. | |
| options := make([]huh.Option[string], len(sortedMatches)) | |
| for i, match := range sortedMatches { | |
| label := match.File | |
| if match.Description != "" { | |
| label = fmt.Sprintf("%s - %s", match.File, match.Description) | |
| } | |
| options[i] = huh.NewOption(label, match.File) | |
| } | |
| var selectedFile string | |
| // Create custom keymap that adds ESC to quit keys. | |
| keyMap := huh.NewDefaultKeyMap() | |
| keyMap.Quit = key.NewBinding( | |
| key.WithKeys("ctrl+c", "esc"), | |
| key.WithHelp("ctrl+c/esc", "quit"), | |
| ) | |
| form := huh.NewForm( | |
| huh.NewGroup( | |
| huh.NewSelect[string](). | |
| Title(fmt.Sprintf("Multiple workflows found with name '%s'. Please choose:", sortedMatches[0].Name)). | |
| Description("Press ctrl+c or esc to exit"). | |
| Options(options...). | |
| Value(&selectedFile), | |
| ), | |
| ).WithKeyMap(keyMap).WithTheme(uiutils.NewAtmosHuhTheme()) | |
| if err := form.Run(); err != nil { | |
| if errors.Is(err, huh.ErrUserAborted) { | |
| return "", errUtils.ErrUserAborted | |
| } | |
| return "", fmt.Errorf("workflow selection failed: %w", err) | |
| } | |
| return selectedFile, nil | |
| } | |
| // promptForWorkflowFile shows an interactive selector for choosing a workflow file. | |
| // Uses the Huh library with Atmos theme (same pattern as identity selector). | |
| func promptForWorkflowFile(matches []WorkflowMatch) (string, error) { | |
| defer perf.Track(nil, "exec.promptForWorkflowFile")() | |
| if len(matches) == 0 { | |
| return "", ErrNoWorkflowFilesToSelect | |
| } | |
| // Check if we're in a TTY environment. | |
| if !term.IsTTYSupportForStdin() || telemetry.IsCI() { | |
| return "", ErrNonTTYWorkflowSelection | |
| } | |
| // Sort matches alphabetically by file name for consistent ordering. | |
| sortedMatches := make([]WorkflowMatch, len(matches)) | |
| copy(sortedMatches, matches) | |
| sort.Slice(sortedMatches, func(i, j int) bool { | |
| return sortedMatches[i].File < sortedMatches[j].File | |
| }) | |
| // Build options for the selector. | |
| // Each option shows the file name with description if available. | |
| options := make([]huh.Option[string], len(sortedMatches)) | |
| for i, match := range sortedMatches { | |
| label := match.File | |
| if match.Description != "" { | |
| label = fmt.Sprintf("%s - %s", match.File, match.Description) | |
| } | |
| options[i] = huh.NewOption(label, match.File) | |
| } | |
| var selectedFile string | |
| // Create custom keymap that adds ESC to quit keys. | |
| keyMap := huh.NewDefaultKeyMap() | |
| keyMap.Quit = key.NewBinding( | |
| key.WithKeys("ctrl+c", "esc"), | |
| key.WithHelp("ctrl+c/esc", "quit"), | |
| ) | |
| form := huh.NewForm( | |
| huh.NewGroup( | |
| huh.NewSelect[string](). | |
| Title(fmt.Sprintf("Multiple workflows found with name '%s'. Please choose:", sortedMatches[0].Name)). | |
| Description("Press ctrl+c or esc to exit"). | |
| Options(options...). | |
| Value(&selectedFile), | |
| ), | |
| ).WithKeyMap(keyMap).WithTheme(uiutils.NewAtmosHuhTheme()) | |
| if err := form.Run(); err != nil { | |
| if errors.Is(err, huh.ErrUserAborted) { | |
| return "", errUtils.ErrUserAborted | |
| } | |
| return "", fmt.Errorf("workflow selection failed: %w", err) | |
| } | |
| return selectedFile, nil | |
| } |
🤖 Prompt for AI Agents
In internal/exec/workflow_utils.go around lines 451 to 508, the function
promptForWorkflowFile is missing the required performance tracking call; add
defer perf.Track(nil, "exec.promptForWorkflowFile")() as the first executable
statement in the function (immediately after the opening brace) so execution is
measured for the whole function; ensure the defer is placed before any early
returns and that perf is imported/available in the file.
Make the workflow `--file` flag optional with auto-discovery and interactive selection. When no workflow file is specified, the system searches across all workflow configuration files and prompts the user to select one if multiple matches are found. Key changes: - Centralize ALL error formatting to main.go, eliminating "deep error printing" pattern - Add descriptive "Workflow Error" title to workflow-specific errors - Use `ui.Infof()` for "Executing command" messages - Wrap command examples in markdown code fences for proper rendering - Use single quotes for shell-safe command hints (handles values with spaces) - Create static error definitions instead of dynamic errors.New() - Convert if-else chains to switch statements for better style - Regenerate all workflow test snapshots to match new output format This follows the same architectural principle as eliminating "deep exits" (os.Exit calls) - we're now eliminating "deep print errors" (CheckErrorAndPrint calls). 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Add GetAliases() method to satisfy updated CommandProvider interface - Update workflow test to reflect auto-discovery behavior - Make workflow file discovery more resilient by skipping invalid files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add comprehensive blog post announcing the workflow file auto-discovery feature that allows users to run workflows without specifying --file flag when workflow names are unique. Highlights: - Better developer experience - faster workflow execution - Backward compatible - existing scripts continue to work - Smart discovery - auto-selects unique workflows, prompts for duplicates - Consistent with other Atmos commands 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove Implementation Details, Migration Guide, and Technical Details sections to make the blog post more concise and user-focused. The post now focuses on: - What's new and why it matters - How it works with clear examples - Backward compatibility assurance - Getting started quickly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Further streamline the blog post by removing the Get Started section. The feature is self-explanatory from the examples, making this section redundant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add extensive test coverage for workflow-related code, significantly improving test coverage for shell completion, auto-discovery, and error handling paths. Coverage improvements: - cmd/workflow/utils.go: 5.88% → 53.5% (+47.6%) - internal/exec/workflow_utils.go: 32.46% → ~70% (+37.5%) - findWorkflowAcrossFiles: 100% coverage - ExecuteDescribeWorkflows: 88.6% coverage New tests added: Shell Completion Tests (15 tests): - Stack flag completion (success & error paths) - List all stacks (success, config errors, invalid stacks) - Identity flag completion (with/without identities, errors) - Workflow name completion (success, multiple args, errors, duplicates) - Helper function tests (addStackCompletion, addIdentityCompletion) Auto-Discovery Tests (4 tests): - Find workflow across files (multiple matches, single match, not found) - Handle ExecuteDescribeWorkflows errors Error Path Tests (3 tests): - Invalid YAML file handling (logged and skipped) - Files without workflows key (logged and skipped) - Empty workflows directory All tests follow project conventions: - Table-driven tests where appropriate - Use existing test fixtures and patterns - Test error paths and edge cases - Proper test isolation with t.TempDir() and t.Setenv() - Clear test names describing behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace CheckErrorAndPrint calls with errUtils.Build() pattern in workflow_utils.go for ErrWorkflowNoSteps and ErrInvalidFromStep. This makes the code testable by returning errors instead of printing and continuing. Update previously skipped tests in workflow_test.go to now test the error paths directly, improving coverage for ExecuteWorkflowCmd. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Regenerate golden snapshots for workflow error tests to reflect the new error formatter output. The errUtils.Build() pattern eliminates duplicate error messages by formatting errors once instead of twice (CheckErrorAndPrint + error return). Update test expectations in workflows.yaml to match new error format with "## Hints" section instead of "### Available steps:". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…aultIdentity After rebasing onto main (PR #1849 workflow redesign): - Add GetFlagsBuilder, GetPositionalArgsBuilder, and GetCompatibilityFlags to WorkflowCommandProvider - Restore checkAndMergeDefaultIdentity function that was removed during conflict resolution - Regenerate snapshots to reflect formatting improvements from previous commits The nested heading format (### Available steps:) from commit 92dc8f9 was already applied during the rebase to pkg/workflow/executor.go where the logic now resides.
osterman
force-pushed
the
osterman/workflow-file-optional
branch
from
cfec0be to
f4aee4a
Compare
Add missing defer perf.Track() call to promptForWorkflowFile function as required by the codebase performance tracking guidelines. Also fix pre-existing lint issues in the files: - Use %w instead of %v for error wrapping (aws_utils.go) - Use sentinel error with ErrorBuilder (workflow_utils.go) - Convert if-else chain to switch statement (workflow_utils.go) - Extract helper functions to reduce nesting complexity (workflow_utils.go) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
what
--fileflag optional with auto-discovery and interactive selectioncmd/workflow/package structure following command registry patternErrNoWorkflowFilesToSelect,ErrNonTTYWorkflowSelection) instead of dynamic errorswhy
references
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
Summary by CodeRabbit
Release Notes
New Features
--fileflag is specified; interactive selection prompts when multiple files define the same workflow.Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.