Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add more descriptive errors on task/workflow run failure #370

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

eugenejahn
Copy link
Contributor

@eugenejahn eugenejahn commented Apr 11, 2022

Signed-off-by: eugenejahn [email protected]

Not sure should I suppose to fix this... I was trying to fix https://github.com/flyteorg/flyteconsole/issues/336, but kind of annoyed by not seeing the error message, so I just fix it. (may not be 100% complete)

Enrich the error message. In launch form, users should see a complete error message from the API response
After fixed:
image

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

I decoded the response from the api and return it as new error. However, one thing I am not quite sure is do we have a decode type for the error message? now, I am just using RawOutputDataConfig type to decode...

also since I don't think we will use decodeErrorResponseMessage in other place, I didn't put in ./utils. Let me know if you want me to put into utils.

Tracking Issue

fixes https://github.com/flyteorg/flyteconsole/issues/337

@anrusina anrusina requested review from a team, ursucarina, anrusina and jsonporter and removed request for a team April 11, 2022 21:49
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #370 (8bb5ddc) into master (5842753) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   66.01%   66.00%   -0.01%     
==========================================
  Files         402      402              
  Lines        8965     8976      +11     
  Branches     1580     1583       +3     
==========================================
+ Hits         5918     5925       +7     
- Misses       3047     3051       +4     
Impacted Files Coverage Δ
src/models/AdminEntity/transformRequestError.ts 76.19% <66.66%> (-13.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4721e84...8bb5ddc. Read the comment docs.

Copy link
Contributor

@anrusina anrusina left a comment

Choose a reason for hiding this comment

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

It looks great! 🚀
Currently we do not have a decode type for such errors, so what you did is probably the best that can be achieved.
I fully agree with you that it is a best place to put it in, as it planned to be used with transformRequestError only at this point.

@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 0.51.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants