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

[UI Feature] Add full-list log output to execution detail panel #744

Merged
merged 7 commits into from
Apr 19, 2023

Conversation

james-union
Copy link
Contributor

@james-union james-union commented Apr 18, 2023

flyteorg/flyte#3592

mono.mov

image

How to test this:
Even though I had a discussion with Dan, but I could not create a workflow to generate the error messages real time so I just test this with some generated text (increasing by time interval on the video)

This PR includes flyteidl version updates (to v1.3.18).

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

Added a new component ScrollableMonospaceText

Tracking Issue

flyteorg/flyte#3592

@james-union james-union self-assigned this Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #744 (6d93cb0) into master (7e008cf) will increase coverage by 0.10%.
The diff coverage is 59.37%.

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   66.40%   66.50%   +0.10%     
==========================================
  Files         474      475       +1     
  Lines       11686    11713      +27     
  Branches     2153     2155       +2     
==========================================
+ Hits         7760     7790      +30     
+ Misses       3926     3923       -3     
Impacted Files Coverage Δ
.../src/components/common/ScrollableMonospaceText.tsx 57.89% <57.89%> (ø)
...rc/components/Executions/ExecutionDetails/utils.ts 74.19% <58.33%> (-12.77%) ⬇️
...cutionDetails/NodeExecutionDetailsPanelContent.tsx 81.57% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@james-union james-union requested a review from jsonporter April 19, 2023 17:24
return reasons.map(
reason =>
(reason.occurredAt
? formatDateUTC(timestampToDate(reason.occurredAt)) + '\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to format the date; just show the text as-is here.

) || []
)
.flat()
.sort((a, b) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We also don't want to do sorting here; just print the array as returned.

taskExecutionDetails?.entities.map(
taskExecution => taskExecution.closure.reason,
taskExecution => taskExecution.closure.reasons || [],
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add a fallback here for cases where users are running older versions of admin; fall back to reason.

@james-union james-union requested a review from jsonporter April 19, 2023 18:38
@jsonporter jsonporter merged commit 5acce2e into master Apr 19, 2023
@jsonporter jsonporter deleted the james/execution-running-log branch April 19, 2023 21:32
ursucarina pushed a commit that referenced this pull request May 10, 2023
* fix: added scrollable mono text for log messages

Signed-off-by: James <[email protected]>

* fix: update flyteidl, use reasons instead of reason

Signed-off-by: James <[email protected]>

* fix: yarn lock file updated

* fix: only messages

* fix: added fallback for reason

Signed-off-by: James <[email protected]>

---------

Signed-off-by: James <[email protected]>
Signed-off-by: Carina Ursu <[email protected]>
@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 1.7.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants