Skip to content

DEV-3557: Fix Workflow Error Message#1404

Merged
aknysh merged 5 commits intomainfrom
DEV-3557/workflow-error-filepath
Aug 21, 2025
Merged

DEV-3557: Fix Workflow Error Message#1404
aknysh merged 5 commits intomainfrom
DEV-3557/workflow-error-filepath

Conversation

@milldr
Copy link
Member

@milldr milldr commented Aug 15, 2025

what

  • Fix workflow path for workflow error message

why

  • If the workflow is in a subdirectory, we need to return the path to the workflow, starting from the workflow base dir

references

Summary by CodeRabbit

  • Bug Fixes

    • Failure messages now show a resume command using the workflow’s relative path (relative to configured base, no extension), ensuring accurate guidance for workflows in subdirectories.
  • Tests

    • Added test case, fixture, and golden snapshot covering workflow failure with a file path and the revised resume command.
  • Chores

    • Added .gitignore rule to exclude generated execution outputs (internal/exec/output.*).

@milldr milldr requested a review from a team as a code owner August 15, 2025 17:00
@github-actions github-actions bot added the size/s Small size PR label Aug 15, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 15, 2025

📝 Walkthrough

Walkthrough

Compute a normalized, relative, extensionless workflow filename for error messages/resume commands; add a failing subdirectory workflow fixture, its stderr golden snapshot and test case validating the resume command; add a .gitignore entry for internal/exec/output.*.

Changes

Cohort / File(s) Summary
Workflow failure handling
internal/exec/workflow_utils.go
Normalize workflow path (convert to slashes, trim configured workflows base path, remove leading slash and extension) and use that relative, extensionless name in resume/failure command text.
Tests
tests/fixtures/scenarios/workflows/stacks/workflows/subdir/test.yaml, tests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden, tests/test-cases/workflows.yaml
Add subdir workflow fixture that intentionally fails; add golden stderr snapshot showing resume command using subdir/test; add test case invoking -f subdir/test.
Repo housekeeping
.gitignore
Add ignore rule internal/exec/output.*.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as Atmos CLI
  participant Exec as Workflow Executor
  participant Err as Error Handler

  User->>CLI: atmos workflow fail -f subdir/test
  CLI->>Exec: Run workflow steps
  Exec->>Exec: Execute step1 (terraform plan ...)
  Exec-->>Err: Step failed
  Err->>Err: Normalize workflow path (toSlash, trim base, strip leading '/', remove extension)
  Err-->>CLI: Construct resume cmd: atmos workflow fail -f subdir/test --from-step step1
  CLI-->>User: Print failure, Explanation, and resume command
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Add missing filepath to workflow error message (DEV-3557)

Out-of-scope changes

Code Change Explanation
Added ignore rule internal/exec/output.* (.gitignore) Repository housekeeping unrelated to DEV-3557's requirement to include the filepath in the error message.

Possibly related PRs

Suggested labels

patch

Suggested reviewers

  • osterman
  • aknysh

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between abe628c and f2fa974.

📒 Files selected for processing (1)
  • internal/exec/workflow_utils.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/exec/workflow_utils.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Build (macos-latest, macos)
  • GitHub Check: Analyze (go)
  • GitHub Check: Lint (golangci)
  • GitHub Check: Summary
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch DEV-3557/workflow-error-filepath

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary or @auto-summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @auto-title anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
tests/fixtures/scenarios/workflows/stacks/workflows/subdir/test.yaml (1)

6-6: Add a trailing newline

YAML linter flags the missing newline at EOF.

Apply this diff:

-      - command: terraform plan mock -s idontexist
+      - command: terraform plan mock -s idontexist
+
tests/test-cases/workflows.yaml (1)

265-286: Test case covers the filepath scenario well

Good addition verifying the resume command uses the relative path without extension. Consider adding another case with a deeper nesting (e.g., subdir/deeper/test) in a follow-up to guard against future regressions.

internal/exec/workflow_utils.go (1)

168-179: Quote --from-step when it contains spaces (defensive UX)

If a step name includes whitespace, the resume command will break. Cheap guard: quote only when needed to avoid snapshot churn.

Apply this diff:

-			resumeCommand := fmt.Sprintf(
-				"%s workflow %s -f %s --from-step %s",
-				config.AtmosCommand,
-				workflow,
-				workflowFileName,
-				step.Name,
-			)
+			stepName := step.Name
+			if strings.ContainsAny(stepName, " \t") {
+				stepName = fmt.Sprintf("%q", stepName)
+			}
+			resumeCommand := fmt.Sprintf(
+				"%s workflow %s -f %s --from-step %s",
+				config.AtmosCommand,
+				workflow,
+				workflowFileName,
+				stepName,
+			)
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6d09d40 and 185a462.

📒 Files selected for processing (5)
  • .gitignore (1 hunks)
  • internal/exec/workflow_utils.go (1 hunks)
  • tests/fixtures/scenarios/workflows/stacks/workflows/subdir/test.yaml (1 hunks)
  • tests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden (1 hunks)
  • tests/test-cases/workflows.yaml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit Inference Engine (.cursor/rules/atmos-rules.mdc)

**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults

Files:

  • internal/exec/workflow_utils.go
🧠 Learnings (4)
📚 Learning: 2025-03-18T12:26:25.329Z
Learnt from: Listener430
PR: cloudposse/atmos#1149
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:7-7
Timestamp: 2025-03-18T12:26:25.329Z
Learning: In the Atmos project, typos or inconsistencies in test snapshot files (such as "terrafrom" instead of "terraform") may be intentional as they capture the exact output of commands and should not be flagged as issues requiring correction.

Applied to files:

  • tests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#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/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden
  • internal/exec/workflow_utils.go
  • tests/test-cases/workflows.yaml
📚 Learning: 2025-02-14T23:12:38.030Z
Learnt from: Listener430
PR: cloudposse/atmos#1061
File: tests/snapshots/TestCLICommands_atmos_vendor_pull_ssh.stderr.golden:8-8
Timestamp: 2025-02-14T23:12:38.030Z
Learning: Test snapshots in the Atmos project, particularly for dry run scenarios, may be updated during the development process, and temporary inconsistencies in their content should not be flagged as issues.

Applied to files:

  • tests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden
📚 Learning: 2025-06-02T14:12:02.710Z
Learnt from: milldr
PR: cloudposse/atmos#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/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden
  • internal/exec/workflow_utils.go
  • tests/test-cases/workflows.yaml
🧬 Code Graph Analysis (4)
.gitignore (1)
tests/cli_test.go (1)
  • applyIgnorePatterns (366-385)
tests/fixtures/scenarios/workflows/stacks/workflows/subdir/test.yaml (1)
internal/exec/workflow_test.go (1)
  • TestExecuteWorkflow (13-234)
internal/exec/workflow_utils.go (2)
pkg/schema/schema.go (1)
  • Workflows (360-363)
internal/exec/workflow_test.go (1)
  • TestExecuteWorkflow (13-234)
tests/test-cases/workflows.yaml (2)
internal/exec/stack_utils_test.go (1)
  • TestBuildTerraformWorkspace (13-101)
internal/exec/workflow_test.go (1)
  • TestExecuteWorkflow (13-234)
🪛 YAMLlint (1.37.1)
tests/fixtures/scenarios/workflows/stacks/workflows/subdir/test.yaml

[error] 6-6: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Lint (golangci)
  • GitHub Check: Analyze (go)
  • GitHub Check: Summary
🔇 Additional comments (2)
.gitignore (1)

48-48: LGTM: ignore rule scoped and harmless

Adding internal/exec/output.* looks safe and prevents committing transient outputs from tests.

tests/snapshots/TestCLICommands_atmos_workflow_failure_with_filepath.stderr.golden (1)

1-24: Snapshot matches the new behavior

The stderr snapshot correctly reflects the updated resume command format with subdir/test.

@codecov
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.32%. Comparing base (1f8c046) to head (f2fa974).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1404      +/-   ##
==========================================
+ Coverage   55.18%   55.32%   +0.13%     
==========================================
  Files         272      272              
  Lines       28487    28491       +4     
==========================================
+ Hits        15721    15763      +42     
+ Misses      10989    10948      -41     
- Partials     1777     1780       +3     
Flag Coverage Δ
unittests 55.32% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coderabbitai[bot]
coderabbitai bot previously approved these changes Aug 15, 2025
@osterman osterman added the patch A minor, backward compatible change label Aug 18, 2025
@aknysh aknysh added no-release Do not create a new release (wait for additional code changes) and removed patch A minor, backward compatible change labels Aug 18, 2025
@aknysh aknysh merged commit ba8288a into main Aug 21, 2025
83 of 84 checks passed
@aknysh aknysh deleted the DEV-3557/workflow-error-filepath branch August 21, 2025 04:06
@github-actions
Copy link

These changes were released in v1.188.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release Do not create a new release (wait for additional code changes) size/s Small size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants