Skip to content

fix: Overwritten workflow failure reason#3583

Merged
didier-wenzek merged 2 commits intothin-edge:mainfrom
didier-wenzek:fix/overwritten-failure-reason
Apr 28, 2025
Merged

fix: Overwritten workflow failure reason#3583
didier-wenzek merged 2 commits intothin-edge:mainfrom
didier-wenzek:fix/overwritten-failure-reason

Conversation

@didier-wenzek
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek commented Apr 28, 2025

Proposed changes

  • Preserve workflow failure reasons if set by a state

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3582

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@didier-wenzek
Preserve workflow failure reason
5a154f1 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek added theme:workflows Theme: Workflow engine topics improvement User value labels Apr 28, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.45283% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/core/tedge_api/src/workflow/state.rs 0.00% 3 Missing ⚠️
crates/core/tedge_api/src/workflow/handlers.rs 97.82% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 28, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
619 0 3 619 100 1h45m56.503192999s

@didier-wenzek
Deprecate methods only used by tests
3b96ade 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
rina23q
rina23q reviewed Apr 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@rina23q rina23q left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Now if workflow has already provided the failure reason, user can check the exit code only in the workflow log file. Would it helpful if we add an exit code field, like {"code": 1} to JSON payload?

@reubenmiller
Copy link
Copy Markdown
Contributor

reubenmiller commented Apr 28, 2025

Now if workflow has already provided the failure reason, user can check the exit code only in the workflow log file. Would it helpful if we add an exit code field, like {"code": 1} to JSON payload?

I'm not necessarily against that proposal, however generally the exit code is already part of the branching logic between states anyway, so the workflow has most of the information it already needs to make the state transition (which is already based on the exit code).

rina23q
rina23q approved these changes Apr 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

Approved

reubenmiller
reubenmiller approved these changes Apr 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approved. I experimented with the new behaviour and it works well.

@didier-wenzek didier-wenzek added this pull request to the merge queue Apr 28, 2025
Merged via the queue into thin-edge:main with commit 79be587 Apr 28, 2025
34 checks passed
@didier-wenzek didier-wenzek deleted the fix/overwritten-failure-reason branch April 28, 2025 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reubenmiller reubenmiller reubenmiller approved these changes

@rina23q rina23q rina23q approved these changes

@albinsuresh albinsuresh Awaiting requested review from albinsuresh albinsuresh is a code owner

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

Assignees

No one assigned

Labels

improvement User value theme:workflows Theme: Workflow engine topics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@didier-wenzek @reubenmiller @rina23q