Conversation
📝 WalkthroughWalkthroughThis change overhauls Atmos workflow error handling and test coverage. Workflow step failure errors now display detailed information about what failed, including the specific command or step. Tests and test fixtures were expanded to cover various workflow error scenarios, and markdown-formatted error messages were introduced for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Workflow Engine
participant TUI
User->>CLI: Run workflow command
CLI->>Workflow Engine: ExecuteWorkflow(...)
Workflow Engine->>TUI: Print step execution info
alt Step fails
Workflow Engine->>TUI: Print markdown error with step name and failure details
Workflow Engine->>CLI: Return structured error
CLI->>User: Exit with error (no duplicate error print)
else Step succeeds
Workflow Engine->>TUI: Print step success info
Workflow Engine->>CLI: Return success
CLI->>User: Exit 0
end
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (4)
tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml (2)
1-7: Consider adding explicit step names.
Adding anamefield to each step in the "pass" workflow can make snapshots and error messages more descriptive:steps: - - command: terraform deploy mock -s nonprod + - name: step1 + command: terraform deploy mock -s nonprod - - command: terraform deploy mock -s prod + - name: step2 + command: terraform deploy mock -s prod
9-15: Consider adding explicit step names.
Similarly, in the "fail" workflow,namekeys for each step would help map failures to human-friendly identifiers rather than relying on auto-generated ones.tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden (1)
3-5: Trim trailing whitespace for robustness.
Lines containing# Errorand the blank separators include extra spaces which may cause snapshot mismatches. Please remove trailing spaces so the golden file matches the actual stderr output exactly.Also applies to: 9-11
tests/fixtures/scenarios/workflows/atmos.yaml (1)
10-10: Remove trailing space.There's a trailing space on this line that should be removed for code cleanliness.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
internal/exec/workflow_utils.go(5 hunks)tests/fixtures/scenarios/workflows/.gitignore(1 hunks)tests/fixtures/scenarios/workflows/atmos.yaml(1 hunks)tests/fixtures/scenarios/workflows/stacks/catalog/mock.yaml(1 hunks)tests/fixtures/scenarios/workflows/stacks/deploy/nonprod.yaml(1 hunks)tests/fixtures/scenarios/workflows/stacks/deploy/prod.yaml(1 hunks)tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_success.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_success.stdout.golden(1 hunks)tests/test-cases/workflows.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/workflows/atmos.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (17)
tests/fixtures/scenarios/workflows/.gitignore (1)
1-10: Ignore Terraform artifacts in workflow fixturesThe ignore patterns correctly exclude Terraform state, lock, plan, and cache files for this test scenario, preventing transient files from polluting the repository.
tests/snapshots/TestCLICommands_atmos_workflow_success.stderr.golden (1)
1-1: Snapshot updated to reflectInfo-level workflow start logThe
Info Executing the workflowline matches the new logging level and format fromworkflow_utils.go. This aligns with the PR objectives.tests/fixtures/scenarios/workflows/stacks/deploy/nonprod.yaml (1)
1-15: New nonprod stack manifest for workflow testsThe manifest defines
stage: nonprod, imports the mock catalog, and correctly overridesfooandbar. Schema header is valid.tests/fixtures/scenarios/workflows/stacks/catalog/mock.yaml (1)
1-10: Catalog component default variables definedThe mock catalog provides default values for
foo,bar, andbazunder the correct schema. This supports environment-specific overrides downstream.tests/fixtures/scenarios/workflows/stacks/deploy/prod.yaml (1)
1-15: New prod stack manifest for workflow testsThe
prodmanifest mirrorsnonprod.yamlwithstage: prodand overrides forfooandbar. Appears consistent and ready for testing.tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml (1)
1-16: YAML test fixture is valid.
The workflows "pass" and "fail" are clearly defined with the expected Terraform commands and align with the new test scenarios.tests/snapshots/TestCLICommands_atmos_workflow_failure.stdout.golden (1)
1-32: Snapshot looks accurate.
ANSI escape codes and Terraform output align with the expected failure run. Ensure there’s no extraneous trailing whitespace and that the leading blank line is intentional.tests/test-cases/workflows.yaml (1)
18-36: Failure test case is correctly defined.
The "atmos workflow failure" block properly specifies the expected stderr patterns and exit code for the failing scenario.tests/snapshots/TestCLICommands_atmos_workflow_success.stdout.golden (1)
1-63: Success snapshot is comprehensive.
The output matches the expected Terraform initialization, planning, warnings, and outputs for both "nonprod" and "prod" workspaces.tests/fixtures/scenarios/workflows/atmos.yaml (1)
22-24: Good log level choice for improved visibility.Setting the log level to Info aligns perfectly with the PR objectives of making workflow execution more visible. This ensures that workflow start and failure messages will be properly displayed.
internal/exec/workflow_utils.go (7)
13-13: Good addition of the charmbracelet log package.This change aligns with the PR objective of standardizing logging across workflows.
36-40: Good implementation of conditional logging.The approach of using different log levels based on dry run status is clean and effective. This allows for appropriate visibility during both actual execution and dry runs.
45-45: Improved workflow visibility through Info level logging.Changing this log from Debug to Info level ensures that workflow execution is always visible to users, fulfilling a key objective of the PR.
69-69: Better structured logging for step execution.Using the structured logging approach with key-value pairs provides clearer context about which step is being executed.
102-102: Clear stack identification in logs.The structured logging for stack information makes it easier to track which stack is being used for each step.
111-117: Comprehensive error logging with structured fields.The detailed debug log with all relevant workflow information will help with troubleshooting failures. Good use of structured logging here.
119-119: Improved error message clarity.The error message now more clearly indicates both the step name and the command that failed, which will help users understand what went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/exec/workflow_utils.go (2)
119-123: Create a constant for 'atmos' string literalThe string literal "atmos" appears multiple times in the codebase. Consider creating a named constant for it.
+const atmosCommand = "atmos" // Later in the code if commandType == "atmos" { - failedCmd = fmt.Sprintf("atmos %s", command) + failedCmd = fmt.Sprintf("%s %s", atmosCommand, command) }🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 121-121:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for it
135-135: Use wrapped static errors instead of dynamic errorsInstead of using dynamic errors with
fmt.Errorf, consider using wrapped static errors for better error handling.+var ( + ErrWorkflowStepFailed = errors.New("workflow step failed") +) // Later in the code -return fmt.Errorf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg) +return errors.Wrap(ErrWorkflowStepFailed, fmt.Sprintf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg))🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 135-135:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg)"
🧹 Nitpick comments (1)
tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml (1)
24-24: Add newline at end of fileYAML files should end with a newline character to comply with standard formatting practices.
echo "This should fail" exit 1 type: shell +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/exec/workflow_utils.go(6 hunks)tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stdout.golden(1 hunks)tests/test-cases/workflows.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stdout.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden
🧰 Additional context used
🪛 GitHub Check: golangci-lint
internal/exec/workflow_utils.go
[failure] 121-121:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for it
[failure] 135-135:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg)"
🪛 YAMLlint (1.35.1)
tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (9)
tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml (1)
1-24: Well-structured test workflows for different scenariosThis test fixture provides a comprehensive set of test workflows covering success and failure cases. The three workflows (pass, fail, and shell-failure) will effectively test the enhanced logging improvements in the workflow execution.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
internal/exec/workflow_utils.go (5)
13-13: Good switch to structured logging packageAdopting the
charmbracelet/logpackage for structured logging aligns with the PR objectives to standardize log handling across workflows.
36-40: Appropriate log level selection based on dry runSetting the log function based on the
dryRunflag ensures commands are always visible before execution when needed, which directly addresses the PR's motivation.
45-45: Improved workflow execution loggingChanging from debug to info level for workflow execution messages ensures better visibility, as specified in the PR objectives.
69-69: Consistent structured logging for workflow stepsUsing the conditional log function with structured fields improves the clarity and consistency of log messages during workflow execution.
Also applies to: 102-102
111-117: Enhanced error context in debug logsThe structured debug log provides comprehensive context about workflow failures, which will be valuable for troubleshooting.
tests/test-cases/workflows.yaml (3)
19-37: Comprehensive test for workflow failure scenarioThis test effectively verifies the improved error messaging for workflow failures, checking that the failing command is clearly displayed along with instructions to resume from the failed step.
39-59: Well-defined shell command failure testThis test ensures proper error handling for shell command failures, which completes the test coverage for the different types of workflow failure scenarios.
1-17:Details
✅ Verification successful
Complete tests for workflow success scenario
The test for successful workflow execution is well-defined, but it's missing an stderr snapshot file.
Since the
snapshot: trueflag is set, both stdout and stderr snapshot files are expected. Please ensure you have aTestCLICommands_atmos_workflow_success.stderr.goldenfile in your snapshot directory to avoid CI failures.
🏁 Script executed:
#!/bin/bash # Check if the stderr snapshot file exists find tests/snapshots -name "TestCLICommands_atmos_workflow_success.stderr.golden"Length of output: 150
All required snapshot files are present
The stderr snapshot file
TestCLICommands_atmos_workflow_success.stderr.goldenwas found undertests/snapshots/. No further action is needed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1229 +/- ##
==========================================
+ Coverage 49.60% 49.93% +0.33%
==========================================
Files 235 235
Lines 25568 25608 +40
==========================================
+ Hits 12683 12788 +105
+ Misses 11242 11166 -76
- Partials 1643 1654 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/exec/workflow_utils.go (2)
113-117: Create a constant for the "atmos" string literalThe string literal "atmos" appears multiple times in the codebase. Creating a named constant would improve maintainability.
+const atmosCmd = "atmos" failedMsg := fmt.Sprintf("\nStep '%s' failed:\n", step.Name) failedCmd := command if commandType == "atmos" { - failedCmd = fmt.Sprintf("atmos %s", command) + failedCmd = fmt.Sprintf("%s %s", atmosCmd, command) }Then update other occurrences at lines 66, 73, and 99 with this constant.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 115-115:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for it
129-129: Consider using wrapped static errors instead of dynamic errorsThe current error formatting uses dynamic errors with fmt.Errorf. Consider using wrapped static errors for better error handling and consistency.
+var ( + ErrWorkflowStepFailed = errors.New("workflow step failed") +) -return fmt.Errorf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg) +return errors.Wrap(ErrWorkflowStepFailed, fmt.Sprintf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg))🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 129-129:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/workflow_utils.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
internal/exec/workflow_utils.go
[failure] 115-115:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for it
[failure] 129-129:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("%s\n%s\n%s", failedMsg, failedCmd, resumeMsg)"
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/workflow_utils.go (4)
13-13: Good update to use structured logging packageThe import of the charmbracelet log package aligns with the PR objective to standardize logging across workflows.
39-39: Improved visibility with Info level loggingElevating the workflow execution notification from Debug to Info level ensures users can always see when a workflow starts, enhancing visibility as intended in the PR objectives.
63-63: Well-structured debug logging for stepsThe structured logging with fields for step index and command provides good context for debugging.
96-96: Clear stack context loggingStack information is properly logged with a structured field providing helpful context.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/exec/workflow_utils.go (2)
50-50: Log level should be Info instead of Debug.Based on the PR objectives, workflow start messages should be changed from Debug to Info level to ensure commands are always visible before execution.
Apply this change:
-l.Debug(fmt.Sprintf("Executing the workflow: workflow=%s file=%s", workflow, workflowPath)) +l.Info("Executing workflow", "workflow", workflow, "file", workflowPath)
127-128: Log level should be Info instead of Debug.The past review comments specifically mentioned that workflow failure messages should be at Info level, not Debug. This ensures failures are visible without debug mode.
Apply this change:
-l.Debug(fmt.Sprintf("Workflow failed: workflow=%s path=%s step=%s command=%s error=%v", - workflow, workflowPath, step.Name, command, err)) +l.Info("Workflow failed", "workflow", workflow, "path", workflowPath, "step", step.Name, "command", command, "error", err)
🧹 Nitpick comments (3)
internal/exec/workflow_utils.go (2)
80-80: Consider using Info level for step execution.Since the PR objectives emphasize visibility of commands before execution, step execution logs might benefit from Info level rather than Debug.
-l.Debug(fmt.Sprintf("Executing workflow step: step=%d command=%s", stepIdx, command)) +l.Info("Executing workflow step", "step", stepIdx, "command", command)
141-141: Add constant for repeated "atmos" string.Static analysis correctly identifies that "atmos" appears multiple times and should be a named constant.
Add this constant at the package level:
+const AtmosCommand = "atmos"Then use it in the code:
-failedCmd = "atmos " + command +failedCmd = AtmosCommand + " " + command🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 141-141:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for ittests/test-cases/workflows.yaml (1)
120-120: Add missing newline at end of file.Static analysis correctly identifies the missing newline character at the end of the file.
Add a newline at the end:
exit_code: 1 +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 120-120: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/exec/workflow_utils.go(4 hunks)tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.golden(1 hunks)tests/test-cases/workflows.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden
- tests/fixtures/scenarios/workflows/stacks/workflows/test.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
tests/test-cases/workflows.yaml
[error] 120-120: no new line character at the end of file
(new-line-at-end-of-file)
🪛 golangci-lint (1.64.8)
internal/exec/workflow_utils.go
[error] 36-36: S1039: unnecessary use of fmt.Sprintf
(gosimple)
🪛 GitHub Check: golangci-lint
internal/exec/workflow_utils.go
[failure] 35-35:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Workflow %s does not have any steps defined", workflow)"
[failure] 36-36:
S1039: unnecessary use of fmt.Sprintf
[failure] 69-69:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Invalid --from-step flag. Workflow %s does not have a step with the name %s", workflow, fromStep)"
[failure] 120-120:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Invalid workflow step type %s", commandType)"
[failure] 141-141:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for it
[failure] 147-147:
error-strings: error strings should not be capitalized or end with punctuation or a newline
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (7)
internal/exec/workflow_utils.go (6)
14-14: Good migration to custom logger.The import change to use the custom logger from
atmos/pkg/loggeraligns well with the PR objectives for standardizing log handling across workflows.
33-38: Excellent error handling improvement.Using
PrintErrorMarkdownAndExitprovides much better user experience with formatted error messages and clear guidance. This aligns perfectly with the PR objectives for enhanced error clarity.🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 36-36: S1039: unnecessary use of fmt.Sprintf
(gosimple)
🪛 GitHub Check: golangci-lint
[failure] 35-35:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Workflow%sdoes not have any steps defined", workflow)"
[failure] 36-36:
S1039: unnecessary use of fmt.Sprintf
44-48: Logger initialization looks solid.Creating the logger from CLI config is the right approach for consistent logging behavior.
66-72: Clean error handling with helpful context.The error message now includes available steps which is much more user-friendly than the previous approach.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 69-69:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Invalid--from-stepflag. Workflow%sdoes not have a step with the name%s", workflow, fromStep)"
145-149: Great improvement in error messaging.The detailed error format with failed command and resume instructions provides excellent user guidance. The markdown formatting makes it very readable.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 147-147:
error-strings: error strings should not be capitalized or end with punctuation or a newline
157-164: Useful helper function for consistent formatting.The
formatListfunction ensures consistent markdown bullet formatting across error messages.tests/test-cases/workflows.yaml (1)
1-120: Comprehensive test coverage for workflow scenarios.The test cases cover all the key workflow scenarios including success, various failure modes, and error conditions. This provides excellent validation of the enhanced error handling and logging behavior.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 120-120: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
internal/exec/workflow_utils.go (1)
157-164: Useful helper function with minor improvement opportunity.The formatList function is a nice addition for consistent markdown formatting.
Consider adding a comment about the trailing newline behavior:
// formatList formats a list of strings into a markdown bullet list. +// Returns a string with each item as a markdown bullet point, ending with a newline. func formatList(items []string) string {tests/test-cases/workflows.yaml (1)
108-130: Fix missing newline at end of file.Good test case but needs the required newline at EOF to satisfy static analysis.
- "• step1" exit_code: 1 +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 130-130: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/exec/workflow_utils.go(4 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.golden(1 hunks)tests/test-cases/workflows.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_no_steps.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/exec/workflow_utils.go (4)
pkg/utils/markdown_utils.go (1)
PrintErrorMarkdownAndExit(87-89)pkg/logger/logger.go (1)
NewLoggerFromCliConfig(44-50)pkg/schema/schema.go (2)
WorkflowStep(487-492)Command(568-578)internal/exec/shell_utils.go (1)
ExecuteShellCommand(21-75)
🪛 GitHub Check: golangci-lint
internal/exec/workflow_utils.go
[failure] 35-35:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("workflow has no steps defined")"
[failure] 69-69:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid from-step flag")"
[failure] 120-120:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("invalid workflow step type")"
[failure] 141-141:
add-constant: string literal "atmos" appears, at least, 4 times, create a named constant for it
[failure] 146-146:
add-constant: string literal "Workflow Error" appears, at least, 4 times, create a named constant for it
[failure] 147-147:
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("workflow step execution failed")"
🪛 YAMLlint (1.37.1)
tests/test-cases/workflows.yaml
[error] 130-130: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
internal/exec/workflow_utils.go (4)
14-14: Good import change for centralized logging.The switch from the charmbracelet log package to the custom logger package aligns well with standardizing logging across the codebase.
44-48: Solid logger initialization approach.Creating the logger from CLI config ensures consistent logging behavior across the application.
80-80: Good step execution logging.The debug message provides helpful context for troubleshooting workflow execution.
113-113: Consistent debug logging pattern.Stack logging follows the same pattern as other debug messages, maintaining consistency.
tests/test-cases/workflows.yaml (4)
1-16: Comprehensive workflow success test.Good baseline test for successful workflow execution with appropriate snapshot configuration.
17-38: Excellent failure scenario coverage.The test properly validates the new error handling with detailed expectations for the markdown-formatted error output.
40-63: Good shell command failure testing.Properly tests shell command failures and verifies the resume command suggestion functionality.
65-83: Solid edge case coverage.Tests the important edge case of workflows with no steps, validating the new error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
internal/exec/workflow_utils.go (6)
60-60: Use structured logging for consistency.The codebase uses structured logging patterns elsewhere. Replace fmt.Sprintf with key-value pairs for better consistency and performance.
- l.Info(fmt.Sprintf("Executing the workflow %s from %s", workflow, workflowPath)) + l.Info("Executing the workflow", "workflow", workflow, "file", workflowPath)
90-90: Use structured logging consistently.Replace sprintf-based logging with structured logging for better consistency with the codebase patterns.
- l.Debug(fmt.Sprintf("Executing workflow step %d: %s, %s", stepIdx, step.Name, command)) + l.Debug("Executing workflow step", "index", stepIdx, "step", step.Name, "command", command)
123-123: Use structured logging consistently.Maintain consistency with structured logging patterns used elsewhere in the codebase.
- l.Debug(fmt.Sprintf("Stack: %s", finalStack)) + l.Debug("Using stack", "stack", finalStack)
137-137: Use structured logging and consider log level.Replace sprintf-based logging with structured logging for consistency. Also consider whether this should be Info level instead of Debug given the PR objectives about making failures visible.
- l.Debug(fmt.Sprintf("Workflow failed: %s", err)) + l.Info("Workflow step failed", "step", step.Name, "command", command, "error", err)
43-48: 🛠️ Refactor suggestionRemove unreachable return statement.
The return statement after
PrintErrorMarkdownAndExitwill never be executed since that function exits the program. This creates unreachable code.u.PrintErrorMarkdownAndExit( WorkflowErrTitle, ErrWorkflowNoSteps, fmt.Sprintf("\n## Explanation\nWorkflow `%s` is empty and requires at least one step to execute.", workflow), ) - return nil // This line will never be reached due to PrintErrorMarkdownAndExit
128-133: 🛠️ Refactor suggestionRemove unreachable return statement.
The return statement after
PrintErrorMarkdownAndExitis unreachable and should be removed.u.PrintErrorMarkdownAndExit( WorkflowErrTitle, ErrInvalidWorkflowStepType, fmt.Sprintf("\n## Explanation\nStep type `%s` is not supported. Each step must specify a valid type. \n### Available types:\n%s", commandType, formatList([]string{"atmos", "shell"})), ) - return nil // This line will never be reached due to PrintErrorMarkdownAndExit
🧹 Nitpick comments (1)
internal/exec/workflow_utils.go (1)
21-21: Fix comment formatting.The comment should end with a period for consistency with Go documentation standards.
-// Static error definitions +// Static error definitions.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 21-21:
Comment should end in a period
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/exec/workflow_utils.go(4 hunks)pkg/config/const.go(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/config/const.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_step_type.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_invalid_from_step.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure_on_shell_command.stderr.golden
- tests/snapshots/TestCLICommands_atmos_workflow_failure.stderr.golden
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/exec/workflow_utils.go (5)
pkg/utils/markdown_utils.go (1)
PrintErrorMarkdownAndExit(87-89)pkg/logger/logger.go (1)
NewLoggerFromCliConfig(44-50)pkg/schema/schema.go (2)
WorkflowStep(487-492)Command(568-578)internal/exec/shell_utils.go (1)
ExecuteShellCommand(21-75)pkg/config/const.go (1)
AtmosCommand(4-4)
🪛 GitHub Check: golangci-lint
internal/exec/workflow_utils.go
[failure] 21-21:
Comment should end in a period
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/workflow_utils.go (2)
167-174: Well-implemented helper function.The formatList function provides clean markdown formatting for error messages and enhances user experience. Good implementation!
21-28: Excellent static error implementation.Great job addressing the static analysis warnings by implementing proper static error variables. This follows Go best practices and improves error handling consistency.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 21-21:
Comment should end in a period
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/exec/helmfile_test.go (2)
47-97: Simplify directory validation logic.The extensive directory validation and logging makes this test quite verbose and harder to maintain. Consider simplifying this logic or extracting it to a helper function if the validation is truly necessary.
For a helmfile version test, basic directory change should suffice:
- // Get absolute path to work directory - workDirAbs, err := filepath.Abs(tt.workDir) - if err != nil { - t.Fatalf("Failed to get absolute path for work directory: %v", err) - } - log.Debug("Source work directory", "dir", workDirAbs) - - // Change to the work directory - if err := os.Chdir(workDirAbs); err != nil { - t.Fatalf("Failed to change directory to %q: %v", workDirAbs, err) - } - - // Log current working directory for debugging - currentDir, err := os.Getwd() - if err != nil { - t.Fatalf("Failed to get current working directory: %v", err) - } - log.Debug("Current working directory", "dir", currentDir) - - // List contents of current directory - entries, err := os.ReadDir(currentDir) - if err != nil { - t.Fatalf("Failed to read directory contents: %v", err) - } - log.Debug("Directory contents", "files", func() []string { - var names []string - for _, entry := range entries { - names = append(names, entry.Name()) - } - return names - }()) - - // Verify the stacks directory exists - stacksDir := filepath.Join(currentDir, "stacks") - if _, err := os.Stat(stacksDir); os.IsNotExist(err) { - t.Fatalf("Stacks directory does not exist at %q", stacksDir) - } - - // List contents of stacks directory - stackEntries, err := os.ReadDir(stacksDir) - if err != nil { - t.Fatalf("Failed to read stacks directory contents: %v", err) - } - log.Debug("Stacks directory contents", "files", func() []string { - var names []string - for _, entry := range stackEntries { - names = append(names, entry.Name()) - } - return names - }()) + // Change to the work directory + if err := os.Chdir(tt.workDir); err != nil { + t.Fatalf("Failed to change directory to %q: %v", tt.workDir, err) + }
14-131: Consider the long-term maintainability of extensive debugging code.The test has grown significantly with debugging infrastructure. If this level of detail is needed for troubleshooting, consider extracting common debugging utilities to reduce duplication across tests.
Is this extensive debugging code intended to be permanent, or is it temporary for troubleshooting specific issues?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/helmfile_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: milldr
PR: cloudposse/atmos#1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.698Z
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
PR: cloudposse/atmos#1229
File: internal/exec/workflow_test.go:0-0
Timestamp: 2025-06-02T14:12:02.698Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/helmfile_test.go (1)
124-124: Good addition for debugging test failures.Logging the command output before assertions is helpful for troubleshooting failed tests.
what
Moved workflow utils and tests into theThis wont work because we depend on shell utils in internal/exec. Revertedpkg/workflowdirectorywhy
For all other commands, we already have the convention of using a directory for a utilities for a command, such as pkg/workflow. Plus we already had that directory, but it only had incomplete testsreferences
screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores